Repository navigation
Handle multiple algorithm clearance/re-entrancy issues - #7635
Merged
Merged
Conversation
jasnell
added this pull request to stack #7531
October 6, 2026 13:12
Contributor
|
Since last review: 0 resolved, 0 still open, 0 new. Not re-run: tests, api-compat, docs, compat-flags, design-simplicity (no author changes in their files since the last review) Reviewed commit: e0e40712 · github run |
guybedford
approved these changes
Oct 7, 2026
guybedford
left a comment
Contributor
There was a problem hiding this comment.
Checked each commit against the spec text and ran //src/tests/streams/transform:all, //src/tests/streams/writable:all and the WPT streams targets locally; all pass.
- Clearing after close()/abort() return matches
WritableStreamDefaultControllerProcessCloseand[[AbortSteps]]. The reorder is safe: the sink algorithms are promise-wrapped with try/catch (the TransformStream's flush/cancel go through theWritableStreamctor, so same wrapping), so nothing can throw between the call and#clearAlgorithms(), and the double-clear when a reentrantsize()errors inside close() is harmless. The "still rejects as closing and close completes" outcome follows fromWritableStreamDefaultWriterWritecheckingCloseQueuedOrInFlightbefore the erroring state andFinishInFlightCloseturning erroring-without-pending-abort into closed. - The
canCloseOrEnqueuepre-check is the right fix for the readable controller's TypeError falling into the size()-error catch. - Step 5.2 is
Throw stream.[[readable]].[[storedError]]with no undefined guard, sothrow undefinedafter a cancel()/terminate() in size() is the spec result.
One trivial nit inline.
jasnell
force-pushed
the
jasnell/ts-streams-writable-algorithm-clearing
branch
3 times, most recently
from
October 8, 2026 12:56
673c3a8 to
2fc8404
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7635 +/- ##
=======================================
Coverage 38.58% 38.58%
=======================================
Files 868 868
Lines 268500 268500
Branches 25405 25405
=======================================
Hits 103599 103599
Misses 150694 150694
Partials 14207 14207 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jasnell
force-pushed
the
jasnell/ts-streams-writable-algorithm-clearing
branch
from
October 8, 2026 15:54
2fc8404 to
a98ec94
Compare
Base automatically changed from
jasnell/ts-streams-writable-float-comment
to
main
October 8, 2026 19:48
The writable controller cleared its algorithms before calling the sink's close() and abort(); the spec clears them after (WritableStreamDefaultControllerProcessClose, [[AbortSteps]]). A write runs size() before its state checks (WritableStreamDefaultWriterWrite step 4), so a writer.write() made from inside sink.close() or sink.abort() never called size() in TS: 0 calls, against 1 in C++ and Node. The same applies to a TransformStream's flush() and cancel(), which run as its writable's close and abort algorithms. The write still rejected as before (closing, or the abort reason). The algorithms are now cleared once the close or abort algorithm has returned. A size() that throws there begins erroring the stream, but the write still rejects as closing (the close in flight is checked first) and the close completes; inside abort() the stream is already errored and the write rejects with the reason. Node agrees with both. Pins: writable reentrancy.js sizeConsultedForWriteInsideSinkClose (parity for the size() call; writable ledger #15: with a throwing size(), C++ rejects the write with size()'s error) and sizeConsultedForWriteInsideSinkAbort (parity; the throwing-size() shape is TS only, since C++ rejects that write with an internal error). Without the source change, both fail, in the TS cells only. Compatibility: the change is behind the experimental typescript_implemented_streams flag.
TransformStreamDefaultController.enqueue() checked only that the readable side was still 'readable'. Once the readable's close is requested with chunks still queued (flush() has settled; the writable's close is still in flight for a few microtasks), the call passed that check, the readable controller's enqueue() threw its own TypeError, and the catch meant for a throwing size() treated it as one: it errored the writable. In that window writer.desiredSize went from 1 to null and writer.ready rejected with the TypeError (the close still completed). C++ and Node throw the TypeError and leave the writable alone. The pre-check is now ReadableStreamDefaultControllerCanCloseOrEnqueue, as the spec has it (TransformStreamDefaultControllerEnqueue step 4), through a new canCloseOrEnqueue member of the readable module's internals for the transform pairs. Pins: transform error-propagation.js enqueueAfterCloseRequestedLeavesWritable (parity; retries the enqueue at successive microtasks to cover the window, which is longer in C++). Without the source change, it fails, in the TS cells only. Compatibility: the change is behind the experimental typescript_implemented_streams flag.
When the readable-side size() throws, TransformStreamDefaultController .enqueue() throws the readable's stored error (spec TransformStreamDefaultControllerEnqueue step 5.2), so that an error() made inside size() wins over size()'s own exception. TS threw the stored error only when it was not undefined, and size()'s exception otherwise. Measured with size() throwing e2 after re-entering the stream, TS / Node: error(undefined) e2 / undefined; readable.cancel() e2 / undefined; terminate() e2 / undefined (the last two close the readable, which then has no stored error). C++ swallows the exception and enqueue() returns. The enqueue now throws the stored error whatever its value, as the spec and Node do (decision: follow the spec text). error(e1) still gives e1, and a size() that throws without re-entering still gives e2. Pins: transform reentrancy.js enqueueThrowsReadableStoredError (transform ledger #20: C++ returns from enqueue(); with no re-entry it leaves the write pending forever, so that shape is TS only). Without the source change, it fails, in the TS cells only. Compatibility: the change is behind the experimental typescript_implemented_streams flag.
jasnell
force-pushed
the
jasnell/ts-streams-writable-algorithm-clearing
branch
from
October 8, 2026 19:49
a98ec94 to
e0e4071
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.
See the individual commits for details