-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
stream: always emit error before close #19836
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
Conversation
mcollina
left a comment
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 |
mcollina
left a comment
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... |
mcollina
left a comment
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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesExtracted from #19828. Fixes an issue from my error-handling pr, #18438 where a stream emits
closebeforeerroron destroy. It should beerrorbeforeclose.