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: prevent memory overflow due to the default strategy #47129

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lilsweetcaligula
Copy link
Contributor

Fixes: #47128

@lilsweetcaligula lilsweetcaligula marked this pull request as ready for review March 17, 2023 10:32
@anonrig anonrig requested a review from ronag March 17, 2023 13:59
@ronag ronag requested a review from jasnell March 17, 2023 14:02
@zizifn
Copy link

zizifn commented Apr 22, 2023

Any update on this fix? I met this issue when converting Socket via Readable.toWeb... Switching back to Readable will not have any memory issues..

@aduh95
Copy link
Contributor

aduh95 commented Sep 20, 2023

Ping @nodejs/streams for reviews

@ronag
Copy link
Member

ronag commented Sep 20, 2023

I think we might need a separate team for web-streams. I have no idea here... 🤷

@MattiasBuelens
Copy link
Contributor

Here's a different idea: when given a non-object mode stream, can we make Readable.toWeb() return a readable byte stream instead, i.e. new ReadableStream({ type: 'bytes' })? Then { highWaterMark } as a strategy would work just fine, plus users would now be able to use a BYOB reader on the returned stream.

I suppose this might behave differently if an encoding was set using new Readable({ encoding }) or readable.setEncoding(encoding). In that case, the Readable will produce string chunks, which ReadableByteStreamController.enqueue() doesn't accept. We could try to reset the readable's encoding when calling toWeb(), so we get the raw bytes back. Or, if all else fails, we could just decode each string chunk when it's received. 🤷

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. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory overflow in Readable.toWeb due to the default strategy
6 participants