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

stream: always emit error before close #19836

Closed

Conversation

mafintosh
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Extracted from #19828. Fixes an issue from my error-handling pr, #18438 where a stream emits close before error on destroy. It should be error before close.

Copy link
Member

@mcollina mcollina left a 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()?

@mafintosh mafintosh force-pushed the fix-stream-destroy-events-order branch 2 times, most recently from 102b6fe to 2f59bb3 Compare April 5, 2018 18:53
@mafintosh
Copy link
Member Author

@mcollina added test, PTAL

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina mcollina added this to the 10.0.0 milestone Apr 5, 2018
@lpinca
Copy link
Member

lpinca commented Apr 5, 2018

@mscdex
Copy link
Contributor

mscdex commented Apr 5, 2018

I think you may need to rebase to include the recent npm version reverts to fix the CI failures?

@addaleax
Copy link
Member

addaleax commented Apr 5, 2018

Rebuilding CI: https://ci.nodejs.org/job/node-test-pull-request/14080/

This should rebase automatically … let’s see what happens

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 6, 2018
@mafintosh mafintosh force-pushed the fix-stream-destroy-events-order branch from 2f59bb3 to dfd2f30 Compare April 6, 2018 10:33
@mafintosh
Copy link
Member Author

Rebased...

@mafintosh
Copy link
Member Author

mafintosh commented Apr 6, 2018

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mafintosh
Copy link
Member Author

Landed in a7c25b7

@mafintosh mafintosh closed this Apr 9, 2018
@mafintosh mafintosh deleted the fix-stream-destroy-events-order branch April 9, 2018 10:32
mafintosh added a commit that referenced this pull request Apr 9, 2018
PR-URL: #19836
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants