stream: add a non-destroying iterator to Readable #38526
Conversation
add a non-destroying iterator to Readable fixes: nodejs#38491
There was a problem hiding this comment.
Good work! I've left a few notes.
| has less then 64KB of data because no `highWaterMark` option is provided to | ||
| [`fs.createReadStream()`][]. | ||
|
|
||
| ##### `readable.iterator([options])` |
There was a problem hiding this comment.
Just a thought: does this have to be in a new method rather than adding a parameter to the existing Symbol.asyncIterator one?
There was a problem hiding this comment.
I'm -0 on naming. I prefer a new method as the Symbol.asyncIterator one has a predefined signature by the standard.
There was a problem hiding this comment.
It doesn't feel right to me if the user has to call [Symbol.asyncIterator]() themselves.
There was a problem hiding this comment.
the
Symbol.asyncIteratorone has a predefined signature by the standard.
Not really, all the standard says it this method is called with no argument (https://tc39.es/ecma262/#sec-getiterator). The standard gives a clear rule for the returned object (https://tc39.es/ecma262/#sec-asynciterable-interface), but not for the function signature. I personally don't feel strongly either way.
There was a problem hiding this comment.
I'd prefer the separate method. The key issue with extending standard-defined APIs is that it makes reasoning about the portability of code far more complicated. A separate method makes it clear. That said, the behavior of the two can be identical such that [Symbol.asyncIterator]() could just defer to readable.iterator() with default arguments.
There was a problem hiding this comment.
That said, the behavior of the two can be identical such that
[Symbol.asyncIterator]()could just defer toreadable.iterator()with default arguments.
Yeah, the reason that one doesn't call the other is because of legacy streams and this. I'd need to use ReflectApply to bind this, and I preferred to have a regular method and send this as the first parameter instead of primordials.
|
I'm a bit torn about this because I think there is a good chance we picked the wrong default for |
|
I think the defaults are currently sound as the developer would need to do something to destroy the stream manually. The current defaults are safe. |
|
So here are the two use cases I have: for await(const chunk of fs.createReadStream('./foo')) { // this should not leak
}
for await(const chunk of someMethodReturningAStream()) { // this should not leak
}On the other hand, I want the following to also work: const stream = fs.createReadSream('./someFile');
for await(const chunk of stream) {
if (isSpecial(chunk)) break;
processFirst(chunk); // e.g. read http headers
}
for await(const chunk of stream) { // continues from where the last for await ended
if (isOtherSpecial(chunk)) break;
processSecond(chunk); // e.g. read http body
} |
One option would be to add a
const stream = fs.createReadStream('./someFile');
stream.setIterationMode({ destroyOnReturn: false, destoryOnError:false });
for await(const chunk of stream) {
if (isSpecial(chunk)) break;
processFirst(chunk); // e.g. read http headers
}
for await(const chunk of stream) { // continues from where the last for await ended
if (isOtherSpecial(chunk)) break;
processSecond(chunk); // e.g. read http body
} |
|
@benjamingr #38526 (comment) requirements are mutually exclusive. |
That's sort of my point - it's the most concise way I could describe the problem. I'm not sure It's possible that this is the best we can do - I just think it's a difficult problem and I want to make sure we're not exploring too few options here. |
|
How could it be on the stream? |
|
@benjamingr Do you have any outstanding objections to this getting merged? |
|
Nope, just uncertainty 😅 |
|
I'd be more comfortable if this was experimental but I'm fine with this landing as stable. |
|
I'd be happy to land it doc-experimental if you prefer @benjamingr? No warnings and we do not backport. |
doc-experimental is good to me. If others feel strongly that this is the right API I'd also happily concede. |
|
@Linkgoron can you add the experimental badge in there? |
|
Added the experimental tag in the method docs |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Landed in df85d37 |
Add a non-destroying async iterator to Readable.
fixes: #38491
@nodejs/streams
A few things that I think might need attention:
createAsyncIteratormethod remove the listeners that it adds after the iteration ends? Currently it does not, but this was done when it essentially always destroyed the stream. Now it does not (although, it would be a bit problematic with Error, as it emits an error next-tick if an error was thrown). I'm not sure if it's that bad, as the listeners are mostly noops anyway.