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
PR-URL: #54206
Fixes: #54205
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
indutny authored and targos committed Oct 2, 2024
1 parent 62bf03b commit f408536
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 0 deletions.
6 changes: 6 additions & 0 deletions lib/internal/webstreams/adapters.js
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ function newReadableStreamFromStreamReadable(streamReadable, options = kEmptyObj
const strategy = evaluateStrategyOrFallback(options?.strategy);

let controller;
let wasCanceled = false;

function onData(chunk) {
// Copy the Buffer to detach it from the pool.
Expand All @@ -448,6 +449,10 @@ function newReadableStreamFromStreamReadable(streamReadable, options = kEmptyObj
streamReadable.on('error', () => {});
if (error)
return controller.error(error);
// Was already canceled
if (wasCanceled) {
return;
}
controller.close();
});

Expand All @@ -459,6 +464,7 @@ function newReadableStreamFromStreamReadable(streamReadable, options = kEmptyObj
pull() { streamReadable.resume(); },

cancel(reason) {
wasCanceled = true;
destroy(streamReadable, reason);
},
}, 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 f408536

Please sign in to comment.