-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: allow readable to end early without error #40881
Conversation
@nodejs/streams |
@benjamingr I think this PR will be necessary for e.g. take(n). |
lib/internal/streams/pipeline.js
Outdated
} else { | ||
ret = makeAsyncIterable(ret); | ||
|
||
} else if (isIterable(ret)) { | ||
finishCount++; | ||
pump(ret, stream, finish); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pump has its own cleanup logic through async iterator.
@nodejs/streams I noticed some other issues while working on this. Please see updated tests. |
return ret; | ||
}, common.mustCall((err, val) => { | ||
assert.strictEqual(err, undefined); | ||
assert.strictEqual(val, 'helloworld'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems to be broken. val
is undefined, I'm not sure why the thrown exception does not make the process exit. I did not investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea.. that's weird. Fixed the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated test looks good, but why the process did not exit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can reproduce the same issue with current Node.js version (v17.2.0) so it is not related to this PR but it is something to investigate and fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the error thrown in the callback is caught and swallowed here
node/lib/internal/streams/pipeline.js
Lines 163 to 164 in 147d23b
} catch (err) { | |
finish(error !== err ? aggregateTwoErrors(error, err) : err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, PTAL, f0ec29b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a test for cf85105?
Landed in 1fa507f |
PR-URL: #40881 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #40881 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #40881 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@ronag do you mind making a backport PR for this for v16.x-staging? It did not land cleanly when pulling into the 16.x release. |
PR-URL: nodejs#40881 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
No description provided.