From 6080362b7df46cc59c83df440eaa3a0a842d0c44 Mon Sep 17 00:00:00 2001 From: Ethan Arrowood Date: Thu, 3 Jul 2025 10:27:03 -0600 Subject: [PATCH 1/3] test: deflake stream-readable-to-web test Co-authored-by: Luigi Pinca --- test/parallel/test-stream-readable-to-web.js | 62 ----------------- test/parallel/test-stream-readable-to-web.mjs | 69 +++++++++++++++++++ 2 files changed, 69 insertions(+), 62 deletions(-) delete mode 100644 test/parallel/test-stream-readable-to-web.js create mode 100644 test/parallel/test-stream-readable-to-web.mjs diff --git a/test/parallel/test-stream-readable-to-web.js b/test/parallel/test-stream-readable-to-web.js deleted file mode 100644 index 753672b509c173..00000000000000 --- a/test/parallel/test-stream-readable-to-web.js +++ /dev/null @@ -1,62 +0,0 @@ -'use strict'; -const common = require('../common'); -if (!common.hasCrypto) { common.skip('missing crypto'); } - -const { Readable } = require('stream'); -const process = require('process'); -const { randomBytes } = require('crypto'); -const assert = require('assert'); - -// Based on: https://github.com/nodejs/node/issues/46347#issuecomment-1413886707 -// edit: make it cross-platform as /dev/urandom is not available on Windows -{ - let currentMemoryUsage = process.memoryUsage().arrayBuffers; - - // We initialize a stream, but not start consuming it - const randomNodeStream = new Readable({ - read(size) { - randomBytes(size, (err, buffer) => { - if (err) { - // If an error occurs, emit an 'error' event - this.emit('error', err); - return; - } - - // Push the random bytes to the stream - this.push(buffer); - }); - } - }); - // after 2 seconds, it'll get converted to web stream - let randomWebStream; - - // We check memory usage every second - // since it's a stream, it shouldn't be higher than the chunk size - const reportMemoryUsage = () => { - const { arrayBuffers } = process.memoryUsage(); - currentMemoryUsage = arrayBuffers; - - assert(currentMemoryUsage <= 256 * 1024 * 1024); - }; - setInterval(reportMemoryUsage, 1000); - - // after 1 second we use Readable.toWeb - // memory usage should stay pretty much the same since it's still a stream - setTimeout(() => { - randomWebStream = Readable.toWeb(randomNodeStream); - }, 1000); - - // after 2 seconds we start consuming the stream - // memory usage will grow, but the old chunks should be garbage-collected pretty quickly - setTimeout(async () => { - // eslint-disable-next-line no-unused-vars - for await (const _ of randomWebStream) { - // Do nothing, just let the stream flow - } - }, 2000); - - setTimeout(() => { - // Test considered passed if we don't crash - process.exit(0); - }, 5000); -} diff --git a/test/parallel/test-stream-readable-to-web.mjs b/test/parallel/test-stream-readable-to-web.mjs new file mode 100644 index 00000000000000..134bd0801f2009 --- /dev/null +++ b/test/parallel/test-stream-readable-to-web.mjs @@ -0,0 +1,69 @@ +import { hasCrypto, skip } from '../common/index.mjs'; +if (!hasCrypto) { skip('missing crypto'); } + +import { Readable } from 'stream'; +import { memoryUsage } from 'process'; +import { randomBytes } from 'crypto'; +import assert from 'assert'; +import { setImmediate } from 'timers/promises'; + +// Based on: https://github.com/nodejs/node/issues/46347#issuecomment-1413886707 +// edit: make it cross-platform as /dev/urandom is not available on Windows + +const MAX_MEM = 256 * 1024 * 1024; // 256 MiB + +function checkMemoryUsage() { + assert(memoryUsage().arrayBuffers < MAX_MEM); +} + +let end = false; + +const timeout = setTimeout(() => { + end = true; +}, 5000); + +const randomNodeStream = new Readable({ + read(size) { + if (end) { + this.push(null); + return; + } + + randomBytes(size, (err, buf) => { + if (err) { + this.destroy(err); + return; + } + + this.push(buf); + }); + } +}); + +randomNodeStream.on('error', (err) => { + clearTimeout(timeout); + assert.fail(err); +}); + +// Before doing anything, make sure memory usage is okay +checkMemoryUsage(); + +// Create stream and check memory usage remains okay + +const randomWebStream = Readable.toWeb(randomNodeStream); + +checkMemoryUsage(); + +try { + // eslint-disable-next-line no-unused-vars + for await (const _ of randomWebStream) { + // Yield event loop to allow garbage collection + await setImmediate(); + // consume the stream + // check memory usage remains okay + checkMemoryUsage(); + } +} catch (err) { + clearTimeout(timeout); + assert.fail(err); +} From 3775dc9ac220f4378a0d3c3ea98fb183345bb5be Mon Sep 17 00:00:00 2001 From: Ethan Arrowood Date: Thu, 3 Jul 2025 13:30:28 -0600 Subject: [PATCH 2/3] test: dynamic import cypto --- test/parallel/test-stream-readable-to-web.mjs | 102 +++++++++--------- 1 file changed, 54 insertions(+), 48 deletions(-) diff --git a/test/parallel/test-stream-readable-to-web.mjs b/test/parallel/test-stream-readable-to-web.mjs index 134bd0801f2009..8c79cdc10d5333 100644 --- a/test/parallel/test-stream-readable-to-web.mjs +++ b/test/parallel/test-stream-readable-to-web.mjs @@ -1,69 +1,75 @@ import { hasCrypto, skip } from '../common/index.mjs'; -if (!hasCrypto) { skip('missing crypto'); } +if (!hasCrypto) { + skip('requires crypto'); +} -import { Readable } from 'stream'; -import { memoryUsage } from 'process'; -import { randomBytes } from 'crypto'; -import assert from 'assert'; -import { setImmediate } from 'timers/promises'; +import { Readable } from 'node:stream'; +import { memoryUsage } from 'node:process'; +import assert from 'node:assert'; +import { setImmediate } from 'node:timers/promises'; +import { test } from 'node:test'; // Based on: https://github.com/nodejs/node/issues/46347#issuecomment-1413886707 // edit: make it cross-platform as /dev/urandom is not available on Windows -const MAX_MEM = 256 * 1024 * 1024; // 256 MiB +test('Stream Readable.toWeb() should not cause memory leak', async function() { + const { randomBytes } = await import('node:crypto'); -function checkMemoryUsage() { - assert(memoryUsage().arrayBuffers < MAX_MEM); -} + const MAX_MEM = 256 * 1024 * 1024; // 256 MiB -let end = false; + function checkMemoryUsage() { + assert(memoryUsage().arrayBuffers < MAX_MEM); + } -const timeout = setTimeout(() => { - end = true; -}, 5000); + let end = false; -const randomNodeStream = new Readable({ - read(size) { - if (end) { - this.push(null); - return; - } + const timeout = setTimeout(() => { + end = true; + }, 5000); - randomBytes(size, (err, buf) => { - if (err) { - this.destroy(err); + const randomNodeStream = new Readable({ + read(size) { + if (end) { + this.push(null); return; } - this.push(buf); - }); - } -}); + randomBytes(size, (err, buf) => { + if (err) { + this.destroy(err); + return; + } -randomNodeStream.on('error', (err) => { - clearTimeout(timeout); - assert.fail(err); -}); + this.push(buf); + }); + } + }); + + randomNodeStream.on('error', (err) => { + clearTimeout(timeout); + assert.fail(err); + }); -// Before doing anything, make sure memory usage is okay -checkMemoryUsage(); + // Before doing anything, make sure memory usage is okay + checkMemoryUsage(); -// Create stream and check memory usage remains okay + // Create stream and check memory usage remains okay -const randomWebStream = Readable.toWeb(randomNodeStream); + const randomWebStream = Readable.toWeb(randomNodeStream); -checkMemoryUsage(); + checkMemoryUsage(); -try { - // eslint-disable-next-line no-unused-vars - for await (const _ of randomWebStream) { - // Yield event loop to allow garbage collection - await setImmediate(); - // consume the stream - // check memory usage remains okay - checkMemoryUsage(); + try { + // eslint-disable-next-line no-unused-vars + for await (const _ of randomWebStream) { + // Yield event loop to allow garbage collection + await setImmediate(); + // consume the stream + // check memory usage remains okay + checkMemoryUsage(); + } + } catch (err) { + clearTimeout(timeout); + assert.fail(err); } -} catch (err) { - clearTimeout(timeout); - assert.fail(err); -} +}); From 5b5f9dc85b6326e3db10eecd594ce28e95f3966d Mon Sep 17 00:00:00 2001 From: Ethan Arrowood Date: Thu, 3 Jul 2025 15:43:55 -0600 Subject: [PATCH 3/3] test: simplify but keep accurate --- test/parallel/test-stream-readable-to-web.mjs | 83 ++++++++----------- 1 file changed, 36 insertions(+), 47 deletions(-) diff --git a/test/parallel/test-stream-readable-to-web.mjs b/test/parallel/test-stream-readable-to-web.mjs index 8c79cdc10d5333..8ff9f2fb16ed1c 100644 --- a/test/parallel/test-stream-readable-to-web.mjs +++ b/test/parallel/test-stream-readable-to-web.mjs @@ -1,65 +1,52 @@ -import { hasCrypto, skip } from '../common/index.mjs'; -if (!hasCrypto) { - skip('requires crypto'); -} - +import { mustCall } from '../common/index.mjs'; import { Readable } from 'node:stream'; import { memoryUsage } from 'node:process'; import assert from 'node:assert'; import { setImmediate } from 'node:timers/promises'; -import { test } from 'node:test'; // Based on: https://github.com/nodejs/node/issues/46347#issuecomment-1413886707 // edit: make it cross-platform as /dev/urandom is not available on Windows -test('Stream Readable.toWeb() should not cause memory leak', async function() { - const { randomBytes } = await import('node:crypto'); - - const MAX_MEM = 256 * 1024 * 1024; // 256 MiB - - function checkMemoryUsage() { - assert(memoryUsage().arrayBuffers < MAX_MEM); - } - - let end = false; +const MAX_MEM = 256 * 1024 * 1024; // 256 MiB - const timeout = setTimeout(() => { - end = true; - }, 5000); - - const randomNodeStream = new Readable({ - read(size) { - if (end) { - this.push(null); - return; - } +function checkMemoryUsage() { + assert(memoryUsage().arrayBuffers < MAX_MEM); +} - randomBytes(size, (err, buf) => { - if (err) { - this.destroy(err); - return; - } +const MAX_BUFFERS = 1000; +let buffersCreated = 0; - this.push(buf); - }); +const randomNodeStream = new Readable({ + read(size) { + if (buffersCreated >= MAX_BUFFERS) { + this.push(null); + return; } - }); - randomNodeStream.on('error', (err) => { - clearTimeout(timeout); - assert.fail(err); - }); + this.push(Buffer.alloc(size)); + buffersCreated++; + } +}); - // Before doing anything, make sure memory usage is okay - checkMemoryUsage(); +randomNodeStream.on('error', (err) => { + assert.fail(err); +}); - // Create stream and check memory usage remains okay +// Before doing anything, make sure memory usage is okay +checkMemoryUsage(); - const randomWebStream = Readable.toWeb(randomNodeStream); +// Create stream and check memory usage remains okay - checkMemoryUsage(); +const randomWebStream = Readable.toWeb(randomNodeStream); - try { +checkMemoryUsage(); + +let timeout; +try { + // Wait two seconds before consuming the stream to see if memory usage increases + timeout = setTimeout(mustCall(async () => { + // Did the stream leak memory? + checkMemoryUsage(); // eslint-disable-next-line no-unused-vars for await (const _ of randomWebStream) { // Yield event loop to allow garbage collection @@ -68,8 +55,10 @@ test('Stream Readable.toWeb() should not cause memory leak', async function() { // check memory usage remains okay checkMemoryUsage(); } - } catch (err) { + }), 2000); +} catch (err) { + if (timeout) { clearTimeout(timeout); - assert.fail(err); } -}); + assert.fail(err); +}