Skip to content

Commit

Permalink
stream: fix cloned webstreams not being unref'd
Browse files Browse the repository at this point in the history
When cloning a `ReadableStream` and `WritableStream`, both use an
internal `MessageChannel` to communicate with the original stream.
Those, however, previously were not unref'd which would lead to the
process not exiting if the stream was not fully consumed.

Fixes: #44985
PR-URL: #51255
Reviewed-By: Matthew Aitken <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
  • Loading branch information
jasnell authored and RafaelGSS committed Jan 2, 2024
1 parent cd61fce commit e443953
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 1 deletion.
2 changes: 2 additions & 0 deletions lib/internal/webstreams/readablestream.js
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,8 @@ class ReadableStream {

[kTransferList]() {
const { port1, port2 } = new MessageChannel();
port1.unref();
port2.unref();
this[kState].transfer.port1 = port1;
this[kState].transfer.port2 = port2;
return [ port2 ];
Expand Down
4 changes: 3 additions & 1 deletion lib/internal/webstreams/transfer.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ class CrossRealmTransformReadableSource {
error);
port.close();
};

port.unref();
}

start(controller) {
Expand Down Expand Up @@ -210,7 +212,7 @@ class CrossRealmTransformWritableSink {
error);
port.close();
};

port.unref();
}

start(controller) {
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/webstreams/writablestream.js
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,8 @@ class WritableStream {

[kTransferList]() {
const { port1, port2 } = new MessageChannel();
port1.unref();
port2.unref();
this[kState].transfer.port1 = port1;
this[kState].transfer.port2 = port2;
return [ port2 ];
Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-webstreams-clone-unref.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
'use strict';

require('../common');
const { ok } = require('node:assert');

// This test verifies that cloned ReadableStream and WritableStream instances
// do not keep the process alive. The test fails if it timesout (it should just
// exit immediately)

const rs1 = new ReadableStream();
const ws1 = new WritableStream();

const [rs2, ws2] = structuredClone([rs1, ws1], { transfer: [rs1, ws1] });

ok(rs2 instanceof ReadableStream);
ok(ws2 instanceof WritableStream);
11 changes: 11 additions & 0 deletions test/parallel/test-whatwg-webstreams-transfer.js
Original file line number Diff line number Diff line change
Expand Up @@ -464,12 +464,23 @@ const theData = 'hello';
tracker.verify();
});
// We create an interval to keep the event loop alive while
// we wait for the stream read to complete. The reason this is needed is because there's
// otherwise nothing to keep the worker thread event loop alive long enough to actually
// complete the read from the stream. Under the covers the ReadableStream uses an
// unref'd MessagePort to communicate with the main thread. Because the MessagePort
// is unref'd, it's existence would not keep the thread alive on its own. There was previously
// a bug where this MessagePort was ref'd which would block the thread and main thread
// from terminating at all unless the stream was consumed/closed.
const i = setInterval(() => {}, 1000);
parentPort.onmessage = tracker.calls(({ data }) => {
assert(isReadableStream(data));
const reader = data.getReader();
reader.read().then(tracker.calls((result) => {
assert(!result.done);
assert(result.value instanceof Uint8Array);
clearInterval(i);
}));
parentPort.close();
});
Expand Down

0 comments on commit e443953

Please sign in to comment.