-
Notifications
You must be signed in to change notification settings - Fork 30k
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: always emit error before close #19836
stream: always emit error before close #19836
Conversation
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 add a test just for destroy()
?
102b6fe
to
2f59bb3
Compare
@mcollina added test, PTAL |
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.
LGTM
I think you may need to rebase to include the recent npm version reverts to fix the CI failures? |
Rebuilding CI: https://ci.nodejs.org/job/node-test-pull-request/14080/ This should rebase automatically … let’s see what happens |
2f59bb3
to
dfd2f30
Compare
Rebased... |
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.
LGTM
Landed in a7c25b7 |
PR-URL: #19836 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesExtracted from #19828. Fixes an issue from my error-handling pr, #18438 where a stream emits
close
beforeerror
on destroy. It should beerror
beforeclose
.