Skip to content

Commit 6b1e7c4

Browse files
jasnellrichardlau
authored andcommitted
stream: guard shareSync() against re-entrant source reads
If a shareSync() source read from a consumer of the same share while producing a value, the nested read re-entered the source iterator. For generators this threw "Generator is already running" from inside the nested read, which recorded that error as the share's source error while the outer read was still in progress, leaving every consumer in an error state. Fail the nested read with ERR_INVALID_STATE before touching the share's state. The source sees the error and may handle it; if it lets it escape, it becomes the source error as with any other source failure. Assisted-by: OpenCode Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #66483 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
1 parent 8a47365 commit 6b1e7c4

2 files changed

Lines changed: 59 additions & 0 deletions

File tree

‎lib/internal/streams/iter/share.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ const {
6060
ERR_INVALID_ARG_TYPE,
6161
ERR_INVALID_ARG_VALUE,
6262
ERR_INVALID_RETURN_VALUE,
63+
ERR_INVALID_STATE,
6364
ERR_OUT_OF_RANGE,
6465
},
6566
} = require('internal/errors');
@@ -548,6 +549,7 @@ class SyncShareImpl {
548549
#sourceExhausted = false;
549550
#sourceError = kNoShareError;
550551
#cancelled = false;
552+
#pulling = false;
551553
#cachedMinCursor = 0;
552554
#cachedMinCursorConsumers = 0;
553555
/** Cumulative byte size of buffered entries */
@@ -747,6 +749,15 @@ class SyncShareImpl {
747749
#pullFromSource() {
748750
if (this.#sourceExhausted || this.#cancelled) return;
749751

752+
// A source that reads from its own share would re-enter its own
753+
// iterator. Fail that read without touching the share's state; the
754+
// source sees the error and may handle it.
755+
if (this.#pulling) {
756+
throw new ERR_INVALID_STATE(
757+
'shareSync() source cannot be read while it is producing a value');
758+
}
759+
760+
this.#pulling = true;
750761
try {
751762
this.#sourceIterator ||= this.#source[SymbolIterator]();
752763

@@ -760,6 +771,8 @@ class SyncShareImpl {
760771
} catch (error) {
761772
this.#sourceError = error;
762773
this.#sourceExhausted = true;
774+
} finally {
775+
this.#pulling = false;
763776
}
764777
}
765778

‎test/parallel/test-stream-iter-share-sync.js‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,50 @@ function testShareSyncDropOldestSplitsOversizedBatches() {
264264
Array.from({ length: slowSeen.length }, (_, i) => 50 - slowSeen.length + i));
265265
}
266266

267+
function testShareSyncReentrantSourceRead() {
268+
const enc = new TextEncoder();
269+
let sibling;
270+
let reentrantError;
271+
function* source() {
272+
yield [enc.encode('a')];
273+
try {
274+
sibling.next();
275+
} catch (err) {
276+
reentrantError = err;
277+
}
278+
yield [enc.encode('b')];
279+
}
280+
const shared = shareSync(source());
281+
const c1 = shared.pull()[Symbol.iterator]();
282+
sibling = shared.pull()[Symbol.iterator]();
283+
284+
assert.deepStrictEqual(c1.next().value, [enc.encode('a')]);
285+
assert.deepStrictEqual(sibling.next().value, [enc.encode('a')]);
286+
// Pulling 'b' runs the source, which tries to read its own share.
287+
assert.deepStrictEqual(c1.next().value, [enc.encode('b')]);
288+
assert.strictEqual(reentrantError?.code, 'ERR_INVALID_STATE');
289+
// The failed re-entrant read left the share intact.
290+
assert.deepStrictEqual(sibling.next().value, [enc.encode('b')]);
291+
assert.strictEqual(c1.next().done, true);
292+
assert.strictEqual(sibling.next().done, true);
293+
}
294+
295+
function testShareSyncReentrantSourceReadUncaught() {
296+
let sibling;
297+
function* source() {
298+
yield [new Uint8Array(1)];
299+
sibling.next();
300+
}
301+
const shared = shareSync(source());
302+
const c1 = shared.pull()[Symbol.iterator]();
303+
sibling = shared.pull()[Symbol.iterator]();
304+
c1.next();
305+
sibling.next();
306+
// The error escapes the source, so it becomes the share's source error.
307+
assert.throws(() => c1.next(), { code: 'ERR_INVALID_STATE' });
308+
assert.throws(() => sibling.next(), { code: 'ERR_INVALID_STATE' });
309+
}
310+
267311
function testShareSyncStringSource() {
268312
const shared = shareSync('hello-sync-share');
269313
const result = textSync(shared.pull());
@@ -281,6 +325,8 @@ Promise.all([
281325
testShareSyncRejectsUnbounded(),
282326
testShareSyncRejectsDropNewest(),
283327
testShareSyncDropOldestSplitsOversizedBatches(),
328+
testShareSyncReentrantSourceRead(),
329+
testShareSyncReentrantSourceReadUncaught(),
284330
testShareSyncStringSource(),
285331
testShareSyncRetainsBufferWhenAllConsumersDetach(),
286332
testShareSyncStrictBackpressureDetaches(),

0 commit comments

Comments
 (0)