stream: 'end' is no longer emitted after 'error' #20334
Labels
stream
Issues and PRs related to the stream subsystem.
Comments
|
@nodejs/streams |
|
/cc @mcollina ... In hindsight, looking it over again, this really should have been semver-major. +1 to reverting in 10.x, keeping it in 11.x, and marking it semver-major. The change itself is the correct thing to do so reverting it in master would not be the right thing. |
|
@mscdex the PR was landed as minor because a) it didn’t break known modules and b) reacting to end after error means an hard-to-understand state. Did this break any module specifically? I’m ok to revert in 10.x.x. |
4 tasks
mcollina
added a commit
to mcollina/node
that referenced
this issue
May 7, 2018
This reverts commit 8f6ab9f. This PR adds _readableState.errorEmitted and add the tracking of it. Fixes: nodejs#6083 See: nodejs#20334 See: nodejs#20449
4 tasks
MylesBorins
pushed a commit
that referenced
this issue
May 8, 2018
This reverts commit 0857790. PR-URL: #20449 Fixes: #20334 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins
pushed a commit
that referenced
this issue
May 8, 2018
This reverts commit 0857790. PR-URL: #20449 Fixes: #20334 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins
pushed a commit
that referenced
this issue
May 9, 2018
This reverts commit 0857790. PR-URL: #20449 Fixes: #20334 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
BYK
pushed a commit
to yarnpkg/yarn
that referenced
this issue
May 23, 2018
**Summary** 1. Added node 10 to CircleCI 2. Fixed a test that was using the `net.createServer` method. This was having issues, likely because of this: nodejs/node#20334 - it now uses a "normal" `http` server instead, serving the same purpose. 3. Changed the `readFirstAvailableStream` method in `util/fs.js`. I'm guessing this is related to the same issue as 2, but there have been quite some changes in `10` that might be causing this - so it's just a guess. Since this piece of code could be clearer and more conceise anyway, I opted to just change it. Now instead of relying on the stream `error` event to check if a file exists, it just explicitly checks it before starting the stream. 4. Upgraded `upath` from `1.0.2` to `1.0.5` which supports node 10 (found through its `engines` field when installing). 5. Upgraded Jest 6. Fixed a bug regarding replacing symlinks to non-existent targets on Windows 7. Removed a redundant test 8. Fixed two test cases that were failing frequently on macOS builds fixes #5761 fixes #5477 **Test plan** CI should be green (including and especially the new node10 job!)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
(I've copied this more or less from the original issue for better visibility)
With node v10 I noticed that
'end'events are no longer emitted after'error'events for streams, which caused breakage at least with my projects and modules. I tracked it down to #20104 which was merged only 5 days ago and never landed in a v9 release. As of this writing, it's not even marked as semver-major which I think is a mistake, as it can clearly break userland.Secondly, there is no indication in the streams documentation that this new behavior is the intended behavior.
With all of these things considered, I think the offending commit should be reverted. At the very least it should be semver-major and delayed for some time to give end users a chance to somehow deal with the change in behavior. Perhaps streams should instead emit
'close'after'error'instead (stream implementors could opt out of this behavior via constructor options)? Something like that would allow for a much smoother transition.The text was updated successfully, but these errors were encountered: