Repository navigation
Conversation
Writable.fromWeb() and Duplex.fromWeb() dispatch writes to the WritableStream directly and complete them through a shared write tracker. A write issued before the WritableStream had started was queued and processed from its start step, so the sink ran in the async context of the code that created the WritableStream, and the tracker invoked the Node.js write callback synchronously from whatever context completed the sink write, so the writes and final() it released ran there too. The promise-based adapter waited for writer.ready and received the result through a reaction, both in the context of the Node.js write. Dispatch writes issued before the start from a writer.ready reaction, failing them if the stream was closed or errored meanwhile, and deliver the write callback in the context of the Node.js write, from a reaction when it completes in another context. Refs: nodejs#66906 Refs: nodejs#65548 Signed-off-by: Matteo Collina <hello@matteocollina.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #67055 +/- ##
==========================================
+ Coverage 90.47% 90.65% +0.17%
==========================================
Files 791 797 +6
Lines 277245 278353 +1108
Branches 53310 53529 +219
==========================================
+ Hits 250834 252329 +1495
+ Misses 16811 16401 -410
- Partials 9600 9623 +23
🚀 New features to boost your workflow:
|
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
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.
Since #65548,
Writable.fromWeb()andDuplex.fromWeb()dispatch writes to theWritableStreamdirectly and complete them through a shared write tracker instead ofwriter.ready+writer.write()promises. That changed the async context in two cases:A write issued before the
WritableStreamhas started is queued and processed from its start step, so the sink runs in the context of the code that created theWritableStream:The tracker invokes the Node.js write callback synchronously from whatever context completed the sink write, so the writes and
final()it releases run there as well (e.g. after a write queued during an asyncstart()).This restores the previous behavior without touching the steady-state path:
writer.readyreaction, as the promise-based adapter did. They fail through the tracker if the stream was closed or errored meanwhile (destroy()during start, covered bytest-webstreams-duplex-fromweb-writev-unhandled-rejection.js);AsyncLocalStorageand after the start, it stays synchronous. Otherwise it is delivered from a reaction registered in that context.The new test covers
write(), corkedwritev(), an asyncstart(),Duplex.fromWeb(), and writes after the start. Expectations match v24.18.0 (with and without--no-async-context-frame) and v26.5.1; four of the five cases fail onmain.Benchmarks (
adapters,encoding-streams,js_transfer; 30 runs): no significant change (writable-from-web+0.7% / -1.1% over two runs).#65548 is in v26.10.0 and on v24.x-staging, so this should follow it to both lines.
While working on this I found a separate, pre-existing bug, also present in v26.5.1:
writer.write()afterwriter.close()while the close is in flight hits an internal assertion inwritableStreamDefaultControllerGetChunkSizeinstead of rejecting. Not addressed here.Refs: #66906
Refs: #65548
Refs: #67052
Refs: #67054
🤖 Generated with Claude Code
https://claude.ai/code/session_01Sfj4nCwaLmXB1NxA7sEJfi