Skip to content

test: deflake stream-readable-to-web test #58948

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 0 additions & 62 deletions test/parallel/test-stream-readable-to-web.js

This file was deleted.

64 changes: 64 additions & 0 deletions test/parallel/test-stream-readable-to-web.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
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';

// 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);
}

const MAX_BUFFERS = 1000;
let buffersCreated = 0;

const randomNodeStream = new Readable({
read(size) {
if (buffersCreated >= MAX_BUFFERS) {
this.push(null);
return;
}

this.push(Buffer.alloc(size));
buffersCreated++;
}
});

randomNodeStream.on('error', (err) => {
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();

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) {
Copy link
Member

@lpinca lpinca Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We start consuming immediately now, this is a significant change. See #46347 (comment).

// Yield event loop to allow garbage collection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this behavior likely relies on the bug described in #54918 - the event loop is not supposed to block on just any garbage collection, which can cause a deadlock, and in the future it might go away (if someone manages to eradicate the deadlock by rewriting the task runner).

Alternatively if there's a specific leaky object with a known class that this tries to test, checkIfCollectableByCounting() from test/common/gc.js might be less flaky. Though I can't really tell whether that's applicable from a look of this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know. I agree that issue seems related here; particularly with the previous heavy use of timers. I believe that the Readable / GC is doing what they are supposed to regarding memory management.

await setImmediate();
// consume the stream
// check memory usage remains okay
checkMemoryUsage();
}
}), 2000);
} catch (err) {
if (timeout) {
clearTimeout(timeout);
}
assert.fail(err);
}
Loading