From d286f1e679955812dfad4d9b0b84518fa981106e Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 16 Jan 2024 16:45:52 +0100 Subject: [PATCH 1/2] Revert "stream: fix cloned webstreams not being unref'd" This reverts commit 4d3923aabaa6d57dc9dc3c554434fa69410c3d55. --- lib/internal/webstreams/readablestream.js | 2 -- lib/internal/webstreams/transfer.js | 4 +--- lib/internal/webstreams/writablestream.js | 2 -- test/parallel/test-webstreams-clone-unref.js | 16 ---------------- test/parallel/test-whatwg-webstreams-transfer.js | 11 ----------- 5 files changed, 1 insertion(+), 34 deletions(-) delete mode 100644 test/parallel/test-webstreams-clone-unref.js diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 5103591312e479..62dfd8a288ab4c 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -598,8 +598,6 @@ class ReadableStream { [kTransferList]() { const { port1, port2 } = new MessageChannel(); - port1.unref(); - port2.unref(); this[kState].transfer.port1 = port1; this[kState].transfer.port2 = port2; return [ port2 ]; diff --git a/lib/internal/webstreams/transfer.js b/lib/internal/webstreams/transfer.js index c4cb4077f88403..136b0d81a99464 100644 --- a/lib/internal/webstreams/transfer.js +++ b/lib/internal/webstreams/transfer.js @@ -143,8 +143,6 @@ class CrossRealmTransformReadableSource { error); port.close(); }; - - port.unref(); } start(controller) { @@ -212,7 +210,7 @@ class CrossRealmTransformWritableSink { error); port.close(); }; - port.unref(); + } start(controller) { diff --git a/lib/internal/webstreams/writablestream.js b/lib/internal/webstreams/writablestream.js index 3bc77fc6fb7067..eea99f321d50c6 100644 --- a/lib/internal/webstreams/writablestream.js +++ b/lib/internal/webstreams/writablestream.js @@ -274,8 +274,6 @@ class WritableStream { [kTransferList]() { const { port1, port2 } = new MessageChannel(); - port1.unref(); - port2.unref(); this[kState].transfer.port1 = port1; this[kState].transfer.port2 = port2; return [ port2 ]; diff --git a/test/parallel/test-webstreams-clone-unref.js b/test/parallel/test-webstreams-clone-unref.js deleted file mode 100644 index 88a9cebd9c3046..00000000000000 --- a/test/parallel/test-webstreams-clone-unref.js +++ /dev/null @@ -1,16 +0,0 @@ -'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); diff --git a/test/parallel/test-whatwg-webstreams-transfer.js b/test/parallel/test-whatwg-webstreams-transfer.js index 7be01c339652c0..01cfaa02ad075e 100644 --- a/test/parallel/test-whatwg-webstreams-transfer.js +++ b/test/parallel/test-whatwg-webstreams-transfer.js @@ -464,23 +464,12 @@ 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(); }); From 6a38f8372b4d6a1331b385dc650e194fe852b840 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 16 Jan 2024 16:49:45 +0100 Subject: [PATCH 2/2] test: add regression test for 51586 Signed-off-by: Matteo Collina --- ...ebstream-structured-clone-no-leftovers.mjs | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 test/parallel/test-webstream-structured-clone-no-leftovers.mjs diff --git a/test/parallel/test-webstream-structured-clone-no-leftovers.mjs b/test/parallel/test-webstream-structured-clone-no-leftovers.mjs new file mode 100644 index 00000000000000..e8af095cce18d5 --- /dev/null +++ b/test/parallel/test-webstream-structured-clone-no-leftovers.mjs @@ -0,0 +1,28 @@ +import '../common/index.mjs'; +import { test } from 'node:test'; +import assert from 'node:assert'; + +test('do not leak promises', async () => { + const buf = new Uint8Array(1); + const readable = new ReadableStream({ + start(controller) { + controller.enqueue(buf); + controller.close(); + } + }); + + const [out1, out2] = readable.tee(); + const cloned = structuredClone(out2, { transfer: [out2] }); + + for await (const chunk of cloned) { + assert.deepStrictEqual(chunk, buf); + } + + for await (const chunk of out2) { + assert.deepStrictEqual(chunk, buf); + } + + for await (const chunk of out1) { + assert.deepStrictEqual(chunk, buf); + } +});