stream: error state timing#30851
Closed
stream: error state timing #30851
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
nodejs-github-bot added console 
ronagforce-pushed the stream-error-cleanup branch 3 times, most recently from 757667b to 8cda582Compare
This comment has been minimized.
This comment has been minimized.
ronagforce-pushed the stream-error-cleanup branch 3 times, most recently from b365b0e to dc03c53Compare
Clean up end simplify errored state. - errorEmitted should be set in the same tick as 'error' is emitted. - errored should be set as soon as an error occurs. - errored should exist on Readable as well. - refactor destroy logic and make it easier to follow.
ronagforce-pushed the stream-error-cleanup branch from dc03c53 to 20a2a29CompareMemberAuthor
since this slightly changes timing it should probably be semver-major, even if it only applies to "internal" state. |
Trott added the semver-major 
Member
lpinca commented Dec 9, 2019
@nodejs/streams |
mcollina approved these changes Dec 9, 2019
Member
mcollina left a comment
mcollina left a comment There was a problem hiding this comment.
Should we change the docs on any of this at all?
LGTM
MemberAuthor
ronag commented Dec 9, 2019
I don't think so. Did you have anything specific in mind? This mostly affects internal details and is how I think it is expected to work. |
Member
mcollina commented Dec 9, 2019
No, not really. |
addaleax added the author ready MemberAuthor
ronag commented Dec 14, 2019
This need another TSC approval. Maybe @addaleax? |
addaleax approved these changes Dec 14, 2019
Collaborator
nodejs-github-bot commented Dec 14, 2019
4 tasks
Collaborator
BridgeAR pushed a commit that referenced this pull request Dec 15, 2019
Clean up end simplify errored state. - errorEmitted should be set in the same tick as 'error' is emitted. - errored should be set as soon as an error occurs. - errored should exist on Readable as well. - refactor destroy logic and make it easier to follow. PR-URL: #30851 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Member
BridgeAR commented Dec 15, 2019
Landed in 67ed526 🎉 |
This was referenced Sep 18, 2021
This was referenced Sep 29, 2022
This was referenced Oct 5, 2022
This was referenced Oct 8, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix, clean up and simplify errored state.
errorEmittedshould be set in the same tick as'error'is emitted.erroredwas not set (if receiving error through_destroy)erroredshould exist onReadableas well.errorEmittedis currently set in a tick before'error'is actually emitted which is a bit confusing (and inconsistent with other event has been emitted state, e.g.endEmitted). If we need to know the synchronous error state we should useerrored.There was a potential race in
console(at least in theory). We want to swallow an error if it's about to be emitted. However, sinceerrorEmittedwas set totruein the tick before the error is actually emitted, the "swallow error" logic might not be applied if the timing is unfortunate.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes