Skip to content

Commit 101e729

Browse files
mcollinaaduh95
mcollina
authored and
aduh95
committed
stream: keep async context of fromWeb writes
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: #66906 Refs: #65548 Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #67055 Refs: #67052 Refs: #67054 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent cdef4ac commit 101e729

3 files changed

Lines changed: 191 additions & 1 deletion

File tree

‎lib/internal/webstreams/adapters.js‎

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ const {
3232
const {
3333
WritableStream,
3434
isWritableStream,
35+
writableStreamDefaultWriterIsStarted,
36+
writableStreamDefaultWriterWriteFailure,
3537
writableStreamDefaultWriterWriteWithRequest,
3638
} = require('internal/webstreams/writablestream');
3739

@@ -62,6 +64,11 @@ const {
6264
kResolvedPromise,
6365
} = require('internal/webstreams/util');
6466

67+
const {
68+
getContinuationPreservedEmbedderData: currentContextFrame,
69+
setContinuationPreservedEmbedderData: setContextFrame,
70+
} = internalBinding('async_context_frame');
71+
6572
const {
6673
isAnyArrayBuffer,
6774
} = require('internal/util/types');
@@ -157,14 +164,17 @@ const noop = () => {};
157164
// batch is in flight at a time, so one callback slot is enough.
158165
// `promise` is non-undefined so writable-stream queue entries are
159166
// discriminated from kNilRequest by `promise === undefined`;
160-
// `failed`/`failure` latch a write that could not proceed.
167+
// `failed`/`failure` latch a write that could not proceed. The callback
168+
// runs in the async context of the Node.js write that set it, as the
169+
// reaction to writer.write() did (see settle()).
161170
function createWriteTracker(getDestination) {
162171
return {
163172
promise: null,
164173
pending: 0,
165174
failed: false,
166175
failure: undefined,
167176
callback: undefined,
177+
contextFrame: undefined,
168178
error: undefined,
169179
hasError: false,
170180
dispatching: false,
@@ -187,6 +197,22 @@ function createWriteTracker(getDestination) {
187197
}
188198
},
189199
settle() {
200+
const contextFrame = this.contextFrame;
201+
this.contextFrame = undefined;
202+
const prior = currentContextFrame();
203+
if (prior === contextFrame) {
204+
this.settleInContext();
205+
return;
206+
}
207+
// Completed from another async context (e.g. a write queued while
208+
// the destination was starting): deliver from a reaction registered
209+
// in the context of the write, once the destination has finished
210+
// processing it.
211+
setContextFrame(contextFrame);
212+
PromisePrototypeThen(kResolvedPromise, () => this.settleInContext());
213+
setContextFrame(prior);
214+
},
215+
settleInContext() {
190216
const callback = this.callback;
191217
this.callback = undefined;
192218
let error;
@@ -224,7 +250,37 @@ function createWriteTracker(getDestination) {
224250
};
225251
}
226252

253+
// A write issued before the destination has started would be queued and
254+
// processed from the start step, in the async context of the code that
255+
// created the WritableStream. Dispatch it from a writer.ready reaction
256+
// instead, as the promise-based adapter did, so the sink runs in the
257+
// context of the Node.js write. The stream may have been closed or
258+
// errored (e.g. by destroy()) by then.
259+
function deferDispatch(writer, tracker, dispatchNow) {
260+
const dispatch = () => {
261+
const failure = writableStreamDefaultWriterWriteFailure(writer);
262+
if (failure !== undefined) {
263+
tracker.failed = true;
264+
tracker.failure = failure;
265+
tracker.settle();
266+
return;
267+
}
268+
dispatchNow();
269+
};
270+
PromisePrototypeThen(writer.ready, dispatch, dispatch);
271+
}
272+
227273
function dispatchWrite(writer, chunk, tracker) {
274+
tracker.contextFrame = currentContextFrame();
275+
if (!writableStreamDefaultWriterIsStarted(writer)) {
276+
deferDispatch(writer, tracker,
277+
() => dispatchWriteNow(writer, chunk, tracker));
278+
return;
279+
}
280+
dispatchWriteNow(writer, chunk, tracker);
281+
}
282+
283+
function dispatchWriteNow(writer, chunk, tracker) {
228284
tracker.dispatching = true;
229285
writableStreamDefaultWriterWriteWithRequest(writer, chunk, tracker);
230286
tracker.dispatching = false;
@@ -233,6 +289,16 @@ function dispatchWrite(writer, chunk, tracker) {
233289
}
234290

235291
function dispatchWritev(writer, chunks, tracker) {
292+
tracker.contextFrame = currentContextFrame();
293+
if (!writableStreamDefaultWriterIsStarted(writer)) {
294+
deferDispatch(writer, tracker,
295+
() => dispatchWritevNow(writer, chunks, tracker));
296+
return;
297+
}
298+
dispatchWritevNow(writer, chunks, tracker);
299+
}
300+
301+
function dispatchWritevNow(writer, chunks, tracker) {
236302
tracker.dispatching = true;
237303
for (let i = 0; i < chunks.length; i++)
238304
writableStreamDefaultWriterWriteWithRequest(writer, chunks[i].chunk, tracker);

‎lib/internal/webstreams/writablestream.js‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1007,6 +1007,23 @@ function writableStreamDefaultWriterWrite(writer, chunk) {
10071007
return promise;
10081008
}
10091009

1010+
function writableStreamDefaultWriterIsStarted(writer) {
1011+
const stream = writer[kState].stream;
1012+
return stream === undefined ||
1013+
stream[kState].controller[kState].started;
1014+
}
1015+
1016+
// Returns the error a write through the writer would fail with, or
1017+
// undefined if the stream still accepts writes.
1018+
function writableStreamDefaultWriterWriteFailure(writer) {
1019+
const streamState = writer[kState].stream[kState];
1020+
const { state } = streamState;
1021+
if (state === 'errored' || state === 'erroring')
1022+
return streamState.storedError;
1023+
if (streamState.closeQueuedOrInFlight || state === 'closed')
1024+
return new ERR_INVALID_STATE.TypeError('WritableStream is closed');
1025+
}
1026+
10101027
// Variant of writableStreamDefaultWriterWrite for pipeTo: the caller
10111028
// provides a shared request object instead of a per-write promise record.
10121029
// `pending` is incremented before the controller write, which can settle
@@ -1458,6 +1475,8 @@ module.exports = {
14581475
writableStreamCloseQueuedOrInFlight,
14591476
writableStreamAddWriteRequest,
14601477
writableStreamDefaultWriterWrite,
1478+
writableStreamDefaultWriterIsStarted,
1479+
writableStreamDefaultWriterWriteFailure,
14611480
writableStreamDefaultWriterWriteWithRequest,
14621481
writableStreamDefaultWriterRelease,
14631482
writableStreamDefaultWriterGetDesiredSize,

‎test/parallel/test-whatwg-webstreams-adapters-async-context.js‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
'use strict';
2+
3+
// Writes through Writable.fromWeb() and Duplex.fromWeb() must reach the
4+
// WritableStream sink in the async context of the Node.js write, also when
5+
// they are issued before the WritableStream has started.
6+
7+
const common = require('../common');
8+
const assert = require('assert');
9+
const { AsyncLocalStorage } = require('async_hooks');
10+
const { Duplex, Writable } = require('stream');
11+
const { setImmediate: tick } = require('timers/promises');
12+
13+
const als = new AsyncLocalStorage();
14+
15+
function store() {
16+
return als.getStore() ?? 'none';
17+
}
18+
19+
function createSink(seen, start) {
20+
return new WritableStream({
21+
start,
22+
write(chunk) { seen.push(`write-${chunk}:${store()}`); },
23+
close() { seen.push(`close:${store()}`); },
24+
});
25+
}
26+
27+
const tests = [];
28+
function test(name, expected, fn) {
29+
tests.push({ name, expected, fn });
30+
}
31+
32+
test('Writable.fromWeb() written before start', [
33+
'write-a:writer', 'write-b:writer', 'close:writer',
34+
], async (seen) => {
35+
const ws = als.run('creator', () => createSink(seen));
36+
const writable = Writable.fromWeb(ws, { decodeStrings: false });
37+
await als.run('writer', () => new Promise((resolve) => {
38+
writable.write('a');
39+
writable.write('b');
40+
writable.end(resolve);
41+
}));
42+
});
43+
44+
test('Writable.fromWeb() writev before start', [
45+
'write-a:writer', 'write-b:writer', 'close:writer',
46+
], async (seen) => {
47+
const ws = als.run('creator', () => createSink(seen));
48+
const writable = Writable.fromWeb(ws, { decodeStrings: false });
49+
await als.run('writer', () => new Promise((resolve) => {
50+
writable.cork();
51+
writable.write('a');
52+
writable.write('b');
53+
writable.uncork();
54+
writable.end(resolve);
55+
}));
56+
});
57+
58+
test('Writable.fromWeb() written during an async start', [
59+
// The WritableStream processes the chunk queued while it was starting
60+
// from its start step. The Node.js write callback still runs in the
61+
// writer's context, so the writes it releases do too.
62+
'write-a:creator', 'write-b:writer', 'close:writer',
63+
], async (seen) => {
64+
const ws = als.run('creator', () => createSink(seen, () => tick()));
65+
const writable = Writable.fromWeb(ws, { decodeStrings: false });
66+
await als.run('writer', () => new Promise((resolve) => {
67+
writable.write('a');
68+
writable.write('b');
69+
writable.end(resolve);
70+
}));
71+
});
72+
73+
test('Writable.fromWeb() written after start', [
74+
'write-a:w1', 'write-b:w2', 'close:ender',
75+
], async (seen) => {
76+
const ws = als.run('creator', () => createSink(seen));
77+
const writable = Writable.fromWeb(ws, { decodeStrings: false });
78+
await tick();
79+
await als.run('w1', () => new Promise((resolve) => writable.write('a', resolve)));
80+
await als.run('w2', () => new Promise((resolve) => writable.write('b', resolve)));
81+
await als.run('ender', () => new Promise((resolve) => writable.end(resolve)));
82+
});
83+
84+
test('Duplex.fromWeb() written before start', [
85+
'write-a:writer', 'close:writer',
86+
], async (seen) => {
87+
const pair = als.run('creator', () => ({
88+
readable: new ReadableStream({ start(c) { c.close(); } }),
89+
writable: createSink(seen),
90+
}));
91+
const duplex = Duplex.fromWeb(pair, { decodeStrings: false });
92+
duplex.resume();
93+
await als.run('writer', () => new Promise((resolve) => {
94+
duplex.write('a');
95+
duplex.end(resolve);
96+
}));
97+
});
98+
99+
(async () => {
100+
for (const { name, expected, fn } of tests) {
101+
const seen = [];
102+
await fn(seen);
103+
assert.deepStrictEqual(seen, expected, name);
104+
}
105+
})().then(common.mustCall());

0 commit comments

Comments
 (0)