Skip to content
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

stream: don't read more than can handle in webstream #46846

Closed
wants to merge 2 commits into from

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Feb 26, 2023

fix #46347

TODO

  • add tests? how could I test that the memory usage is not rising? 🤔

Cc @mcollina

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Feb 26, 2023
}

let chunk;
while ((chunk = streamReadable.read(controller.desiredSize)) !== null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to make sure controller.desiredSize must be less than 1GB as the docs specify

[...] The size argument must be less than or equal to 1 GiB. [...]

from readable.read([size]) - Node Docs

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I don't specify size argument here, in case the desiredSize is really big?

@rluvaton
Copy link
Member Author

rluvaton commented Feb 26, 2023

I think the current behavior in the failing test should be the expected one, why would we need 2 reads?

the failed test is:

{
const duplex = new PassThrough();
const {
readable,
writable,
} = newReadableWritablePairFromDuplex(duplex);
duplex.on('data', common.mustCall(2));
duplex.on('close', common.mustCall());
duplex.on('end', common.mustCall());
duplex.on('finish', common.mustCall());
const writer = writable.getWriter();
const reader = readable.getReader();
const ec = new TextEncoder();
const dc = new TextDecoder();
Promise.all([
writer.write(ec.encode('hello')),
reader.read().then(common.mustCall(({ done, value }) => {
assert(!done);
assert.strictEqual(dc.decode(value), 'hello');
})),
reader.read().then(common.mustCall(({ done, value }) => {
assert(!done);
assert.strictEqual(dc.decode(value), 'there');
})),
writer.write(ec.encode('there')),
writer.close(),
reader.read().then(common.mustCall(({ done, value }) => {
assert(done);
assert.strictEqual(value, undefined);
})),
]).then(common.mustCall());
}

@rluvaton
Copy link
Member Author

rluvaton commented Mar 7, 2023

ping @mcollina

@mcollina
Copy link
Member

mcollina commented Mar 8, 2023

In don't know. I would need to dig deep into this and currently do not have time. Maybe @jasnell has some ideas.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

This is correct but it needs a test

@aduh95
Copy link
Contributor

aduh95 commented May 21, 2024

It looks like this still needs a test.

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label May 21, 2024
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

Copy link
Contributor

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

@github-actions github-actions bot closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. stalled Issues and PRs that are stalled. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readable.toWeb seems to load file contents to memory
4 participants