Repository navigation
Start a native source pull or sink write before marking it in flight - #7646
Merged
Merged
Conversation
Contributor
|
Since last review: 0 resolved, 0 still open, 0 new. Reviewed commit: 6edb0715 · github run |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
guybedford
approved these changes
Oct 7, 2026
guybedford
left a comment
guybedford
left a comment
Contributor
There was a problem hiding this comment.
Ran js-readable-stream-test@{,all-autogates}, js-writable-stream-test@{,all-autogates}, the readable/writable TS suites and //src/wpt:streams-ts@ locally; all pass. Also reverted the two source files to the base branch and confirmed both new KJ tests fail with the stale already in flight error, and pass again with the change.
The reorder is sound: the IoOwn derefs are the only thing that can throw cross-request, nothing between the deref and the flag runs JS, tryGetLength → tryRead order is preserved, and moving onChunkEnqueued keeps it balanced with the onChunkDequeued in the awaitIo continuation. It also covers a synchronously throwing tryRead/write, which is a nice side effect.
The frozen null-proto cpp_exports table turns a missing name into a hard KJ_REQUIRE_NONNULL failure in getCppExport rather than an Object.prototype hit; the test covers prototype, planted-name lookup and frozenness.
jasnell
force-pushed
the
jasnell/ts-streams-bridge-inflight-flags
branch
from
October 7, 2026 22:18
5566b3f to
c8981c7
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7646 +/- ##
========================================
Coverage 38.61% 38.62%
========================================
Files 868 871 +3
Lines 268517 268972 +455
Branches 25432 25466 +34
========================================
+ Hits 103676 103878 +202
- Misses 150619 150851 +232
- Partials 14222 14243 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jasnell
force-pushed
the
jasnell/ts-streams-bridge-inflight-flags
branch
6 times, most recently
from
October 8, 2026 23:23
6a6090a to
e842024
Compare
Base automatically changed from
jasnell/ts-streams-compression-drain-cancel
to
main
October 9, 2026 13:38
ReadableStreamNativeSource and WritableStreamNativeSink set pullInFlight or writeInFlight before dereferencing their IoOwns, which throws when called from a request other than the one that created them. The pull or write then failed with "Cannot perform I/O on behalf of a different request", but the flag stayed set, so every later pull threw "pull() is already in flight." and every later write threw "write() is already in flight.". A cancel would defer the source's release to a read that never started, a tee would wait on it, and an abort or pipeFrom would treat the sink as busy. The sink's observer also counted a chunk that never passed through. Today a throwing hook errors the TS stream, so the JS layer does not call the source or sink again; the stale flag is visible only from C++. With the sync fast-path autogate on, the fast path's own catch releases the source or sink first and masks it. Both now dereference the IoOwns and start the read or write first, and only then set the flag (and, for the sink, count the chunk). Pins: js-readable-stream-test.c++ "ReadableStreamNativeSource pull from another request leaves no pull in flight" (default and BYOB pulls) and js-writable-stream-test.c++ "WritableStreamNativeSink write from another request leaves no write in flight": two attempts from a second request; the first reports the cross-request error, and neither reports an operation in flight. Without the source change, both fail in their default variants (all-autogates passes through the fast path). Compatibility: the change is behind the experimental typescript_implemented_streams flag (the native source and sink back only TS streams).
The C++ bridge fetches every TypeScript streams entry point with getCppExport(), an ordinary property get on the webstreams/cpp_exports module's exports. That object was a plain object literal spreading the two frozen halves (readable.ts and writable.ts cppExports): unfrozen, and inheriting from Object.prototype. Every name the bridge asks for is an own property today, so nothing resolves through the prototype now, and user code cannot reach the table itself (it lives in the bootstrap's C++-side module cache). But a name that went missing, through a rename or a typo, would resolve to whatever user code had put on Object.prototype, and the bridge would call it with internal objects. The table is now frozen and has a null prototype, so such a lookup fails the bridge's KJ_REQUIRE_NONNULL instead. Pin: js-writable-stream-test.c++ "The cpp_exports table has a null prototype and is frozen" checks the prototype, that a function planted on Object.prototype under a missing name is not returned by getCppExport(), and that a new property does not stick. Without the source change, all three assertions fail. The webstreams AGENTS.md key rules record both properties. Compatibility: no user-visible change; the table is internal to the experimental typescript_implemented_streams implementation.
jasnell
force-pushed
the
jasnell/ts-streams-bridge-inflight-flags
branch
from
October 9, 2026 13:41
e842024 to
662d443
Compare
jasnell
force-pushed
the
jasnell/ts-streams-bridge-inflight-flags
branch
from
October 9, 2026 16:16
4cfff4b to
6edb071
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Multiple TS streams bridge updates. See individual commits for details.