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

Add non-destroying AsyncIterator to Readable streams #38491

Closed
Linkgoron opened this issue Apr 30, 2021 · 4 comments
Closed

Add non-destroying AsyncIterator to Readable streams #38491

Linkgoron opened this issue Apr 30, 2021 · 4 comments
Labels
feature request Issues that request new features to be added to Node.js. stream Issues and PRs related to the stream subsystem.

Comments

@Linkgoron
Copy link
Member

Linkgoron commented Apr 30, 2021

Is your feature request related to a problem? Please describe.
The current async-iterator on Readable streams always destroys the stream, when you break or return or an error is thrown. Specifically, this means that if someone wants to iterate over the same stream in different places (for example iterate until the end of part one... then do some things, continue iterating later etc) this is impossible - you have to have exactly one for await...of that you work inside of. In addition, there are other scenarios like Request where iterating over the request ends up closing the response (which might specifically be a bug in the Request iterator, and not really related to destroying the request) and this suggestion would allow bypassing it for other use-cases without adding special specific use-case code (I'm not sure if this is an issue with Duplex streams in general?).

Describe the solution you'd like
I'd like to create another async-iterator (in addition to the existing one) that would give users the option opt in/out of destroying the stream non-error ending of the iteration (any maybe also on stream-error?). The naming could be very explicit, to let users know that they've decided to opt-out of the default behavior, or if an explicit name is not required just added as parameters to the Symbol.asyncIterator method.

Something like this (I haven't thought a lot about the naming):

for await (const chunk of readable.nonDestroyingIterator({ destroyOnError: true, destroyOnReturn: false}) {
}

Looking around createAsyncIterator in readable.js, this wouldn't be too difficult to add, and I would be happy to provide a PR for this.

Describe alternatives you've considered
Building another iterator myself on existing streams.

@Linkgoron Linkgoron added stream Issues and PRs related to the stream subsystem. feature request Issues that request new features to be added to Node.js. labels Apr 30, 2021
@addaleax
Copy link
Member

addaleax commented May 1, 2021

Definitely 👍 to this, I've also been in situations where I've piped a stream to a dummy PassThrough just so I could iterate over it without destroying the original stream.

@misos1
Copy link

misos1 commented May 2, 2021

In addition, there are other scenarios like Request where iterating over the request ends up closing the response

Actually Request is not destroyed when for await does not break or throw (but for example file stream is) if you meant that:

let http = require("http");

let server = http.createServer(async function(req, res)
{
	for await (let chunk of req)
	{
		await new Promise(r => setTimeout(r, 1000));
	}
	console.log("destroyed", req.destroyed);
	res.end("ok");
});

(async function()
{
	await new Promise(resolve => server.listen(resolve));
	let req = http.request({ port: server.address().port, method: "post" }).end("abc");
	let res = await new Promise((resolve, reject) => req.on("response", resolve).on("error", reject));
	let str = "";
	for await (let chunk of res) str += chunk;
	console.log("client received:", str);
	server.close();
}());
destroyed false
client received: ok

@ronag
Copy link
Member

ronag commented May 2, 2021

for await (const chunk of readable.iterator({ destroyOnError: true, destroyOnReturn: false }) {
}

@ronag
Copy link
Member

ronag commented May 3, 2021

Because it's a generic method

Linkgoron added a commit to Linkgoron/node that referenced this issue May 3, 2021
add a non-destroying iterator to Readable

fixes: nodejs#38491
danielleadams pushed a commit that referenced this issue May 31, 2021
add a non-destroying iterator to Readable

fixes: #38491

PR-URL: #38526
Fixes: #38491
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants