Skip to content

Commit

Permalink
lib: fix unhandled errors in webstream adapters
Browse files Browse the repository at this point in the history
WebStream's Readable controller does not tolerate `.close()` being
called after an `error`. However, when wrapping a Node's Readable stream
it is possible that the sequence of events leads to `finished()`'s
callback being invoked after such `error`.

In order to handle this, in this change we call the `finished()` handler
earlier when controller is canceled, and always handle this as an error
case.

Fix: #54205
  • Loading branch information
indutny committed Aug 4, 2024
1 parent 67f7137 commit de63904
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 2 deletions.
8 changes: 6 additions & 2 deletions lib/internal/webstreams/adapters.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ const {
createDeferredPromise,
kEmptyObject,
normalizeEncoding,
once,
} = require('internal/util');

const {
Expand Down Expand Up @@ -471,7 +472,7 @@ function newReadableStreamFromStreamReadable(streamReadable, options = kEmptyObj

streamReadable.pause();

const cleanup = finished(streamReadable, (error) => {
const onFinished = once((error) => {
error = handleKnownInternalErrors(error);

cleanup();
Expand All @@ -482,6 +483,7 @@ function newReadableStreamFromStreamReadable(streamReadable, options = kEmptyObj
return controller.error(error);
controller.close();
});
const cleanup = finished(streamReadable, onFinished);

streamReadable.on('data', onData);

Expand All @@ -491,7 +493,9 @@ function newReadableStreamFromStreamReadable(streamReadable, options = kEmptyObj
pull() { streamReadable.resume(); },

cancel(reason) {
destroy(streamReadable, reason);
const error = reason || new ERR_STREAM_PREMATURE_CLOSE();
onFinished(error);
destroy(streamReadable, error);
},
}, strategy);
}
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-stream-readable-from-web-termination.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
'use strict';
require('../common');
const { Readable } = require('stream');

{
const r = Readable.from(['data']);

const wrapper = Readable.fromWeb(Readable.toWeb(r));

wrapper.on('data', () => {
// Destroying wrapper while emitting data should not cause uncaught
// exceptions
wrapper.destroy();
});
}
12 changes: 12 additions & 0 deletions test/parallel/test-stream-readable-to-web-termination.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
'use strict';
require('../common');
const { Readable } = require('stream');

{
const r = Readable.from([]);
// Cancelling reader while closing should not cause uncaught exceptions
r.on('close', () => reader.cancel());

const reader = Readable.toWeb(r).getReader();
reader.read();
}

0 comments on commit de63904

Please sign in to comment.