stream: address stream/iter review feedback · nodejs/node@82ae196 · GitHub
Skip to content

Commit 82ae196

Browse files
jasnelladuh95
authored andcommitted
stream: address stream/iter review feedback
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode PR-URL: #65652 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 3b37b48 commit 82ae196

5 files changed

Lines changed: 63 additions & 9 deletions

File tree

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

Lines changed: 12 additions & 8 deletions

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ const {
1212
} = primordials;
1313

1414
const {
15+
isConsumerReturnError,
1516
push,
1617
} = require('internal/streams/iter/push');
1718
const {
@@ -99,7 +100,11 @@ async function closeDuplexChannel(writer, closeIterator) {
99100
const returnPromise = closeIterator.return();
100101

101102
if (endPromise !== undefined) {
102-
await SafePromiseAllReturnVoid([endPromise, returnPromise]);
103+
try {
104+
await SafePromiseAllReturnVoid([endPromise, returnPromise]);
105+
} catch (error) {
106+
if (!isConsumerReturnError(error)) throw error;
107+
}
103108
} else {
104109
await returnPromise;
105110
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ const {
1111
PromiseReject,
1212
PromiseResolve,
1313
PromiseWithResolvers,
14+
SafeWeakSet,
1415
Symbol,
1516
SymbolAsyncDispose,
1617
SymbolAsyncIterator,
@@ -55,6 +56,11 @@ const {
5556
} = require('internal/streams/iter/ringbuffer');
5657

5758
const kNoFailReason = Symbol('kNoFailReason');
59+
const consumerReturnErrors = new SafeWeakSet();
60+
61+
function isConsumerReturnError(error) {
62+
return consumerReturnErrors.has(error);
63+
}
5864

5965
function raceEndWithSignal(promise, signal) {
6066
if (!signal) return promise;
@@ -471,6 +477,7 @@ class PushQueue {
471477
if (this.#consumerState !== 'active') return;
472478
this.#consumerState = 'returned';
473479
const error = new ERR_INVALID_STATE.TypeError('Stream closed by consumer');
480+
consumerReturnErrors.add(error);
474481
this.#terminateWriterFromConsumer(error);
475482
this.#resolvePendingReads();
476483
// Resolve pending drains with false - no more data will be consumed
@@ -791,5 +798,6 @@ function push(...args) {
791798
}
792799

793800
module.exports = {
801+
isConsumerReturnError,
794802
push,
795803
};

‎test/parallel/test-stream-iter-consumers-merge.js‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,27 @@ async function testMergeBreakWithCleanupError() {
447447
);
448448
}
449449

450+
async function testMergeMultiSourceBreakWithCleanupError() {
451+
async function* failingReturnSource() {
452+
try {
453+
yield [new TextEncoder().encode('data')];
454+
} finally {
455+
await Promise.resolve();
456+
throwInFinally('async cleanup on break');
457+
}
458+
}
459+
460+
await assert.rejects(
461+
async () => {
462+
// eslint-disable-next-line no-unused-vars
463+
for await (const _ of merge(failingReturnSource(), from('other'))) {
464+
break;
465+
}
466+
},
467+
{ message: 'async cleanup on break' },
468+
);
469+
}
470+
450471
Promise.all([
451472
testMergeTwoSources(),
452473
testMergeSingleSource(),
@@ -468,4 +489,5 @@ Promise.all([
468489
testMergeCleanupErrorOnly(),
469490
testMergePrimaryErrorPrecedesCleanupError(),
470491
testMergeBreakWithCleanupError(),
492+
testMergeMultiSourceBreakWithCleanupError(),
471493
]).then(common.mustCall());

‎test/parallel/test-stream-iter-duplex.js‎

Lines changed: 15 additions & 0 deletions

0 commit comments

Comments
 (0)