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: ensure writable.destroy() emits error once #26057

Closed
wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Feb 12, 2019

Prevent the 'error' event from being emitted multiple times if
writable.destroy() is called with an error before the _destroy()
callback is called.

Emit the first error, discard all others.

Fixes: #26015
Refs: #20745 (comment)

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

Prevent the `'error'` event from being emitted multiple times if
`writable.destroy()` is called with an error before the `_destroy()`
callback is called.

Emit the first error, discard all others.

Fixes: nodejs#26015
@lpinca
Copy link
Member Author

lpinca commented Feb 12, 2019

This does not fix the issue for readable only streams.

@addaleax addaleax added the stream Issues and PRs related to the stream subsystem. label Feb 13, 2019
@addaleax
Copy link
Member

@nodejs/streams

@lpinca
Copy link
Member Author

lpinca commented Mar 2, 2019

cc: @nodejs/streams
CI: https://ci.nodejs.org/job/node-test-pull-request/21114/

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 5, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

Should we actually allow calling destroy() more than once in the first place?

@mcollina
Copy link
Member

mcollina commented Mar 5, 2019

Should we actually allow calling destroy() more than once in the first place?

Yes. This is actually an use case of the API (historically).

@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

Landed in 49f1bb9 🎉

@BridgeAR BridgeAR closed this Mar 5, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 5, 2019
Prevent the `'error'` event from being emitted multiple times if
`writable.destroy()` is called with an error before the `_destroy()`
callback is called.

Emit the first error, discard all others.

PR-URL: nodejs#26057
Fixes: nodejs#26015
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@lpinca lpinca deleted the gh-26015 branch March 6, 2019 06:37
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
Prevent the `'error'` event from being emitted multiple times if
`writable.destroy()` is called with an error before the `_destroy()`
callback is called.

Emit the first error, discard all others.

PR-URL: nodejs#26057
Fixes: nodejs#26015
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
Prevent the `'error'` event from being emitted multiple times if
`writable.destroy()` is called with an error before the `_destroy()`
callback is called.

Emit the first error, discard all others.

PR-URL: #26057
Fixes: #26015
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
lpinca added a commit to lpinca/node that referenced this pull request Jun 1, 2019
Prevent the `'error'` event from being emitted multiple times if
`writable.destroy()` is called with an error before the `_destroy()`
callback is called.

Emit the first error, discard all others.

PR-URL: nodejs#26057
Backport-PR-URL: nodejs#28000
Fixes: nodejs#26015
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Sep 19, 2019
Prevent the `'error'` event from being emitted multiple times if
`writable.destroy()` is called with an error before the `_destroy()`
callback is called.

Emit the first error, discard all others.

PR-URL: #26057
Backport-PR-URL: #28000
Fixes: #26015
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Sep 19, 2019
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. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The 'error' event can be emitted more than once when using writable.destroy()
7 participants