stream: don't emit end after close #33076

ronag · 2020-04-26T09:39:45Z

Readable stream could emit 'end' after 'close'.

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

ronag · 2020-04-26T09:41:46Z

test/parallel/test-stream-readable-destroy.js

@@ -113,7 +113,7 @@ const assert = require('assert');
  read.destroy();

  read.removeListener('end', fail);
-  read.on('end', common.mustCall());
+  read.on('end', common.mustNotCall());


NOTE, possible breaking change

ronag · 2020-04-26T09:41:49Z

test/parallel/test-stream-duplex-destroy.js

@@ -124,7 +124,7 @@ const assert = require('assert');

  duplex.removeListener('end', fail);
  duplex.removeListener('finish', fail);
-  duplex.on('end', common.mustCall());
+  duplex.on('end', common.mustNotCall());


NOTE, possible breaking change

ronag · 2020-04-26T09:42:53Z

Please note that this can have breaking consequences. Though this PR ensures "proper" behaviour the ecosystem might make incorrect assumptions that this PR breaks.

Would appreciate some feedback and thoughts on the risks. @nodejs/streams @mafintosh

nodejs-github-bot · 2020-04-26T09:43:32Z

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

mcollina

I have a feeling this would be too breaking.

ronag · 2020-04-26T13:25:57Z

I have a feeling this would be too breaking.

I suspected soo. Anyway, we are aware of this issue and maybe we can revisit this at a later point. We've got quite a lot of recent semver-major stuff so it might be a good idea to wait regardless.

Should I document this "known issue" somewhere somehow?

mcollina · 2020-04-26T13:32:35Z

Documenting it's a good idea.

vweevers · 2020-04-26T13:57:16Z

On the other hand, the documentation has stated for 5 years now (db4cb33) that the close event "indicates that no more events will be emitted".

mcollina · 2020-04-26T22:27:27Z

I would say let’s fix this. It would be good if we had an opt-out flag.

ronag · 2020-04-27T06:36:57Z

It would be good if we had an opt-out flag.

I'm a little unsure about the value of this. If a library breaks because of this (which is still unlikely, but possible) it should not be much more difficult to actually fix the issue rather than having to add a flag?

Would it be an alternative to begin with no adding a flag, and then add it if this turns out to be a problem?

nodejs-github-bot · 2020-04-27T07:32:11Z

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

mcollina · 2020-04-27T11:24:31Z

On the other hand, the documentation has stated for 5 years now (db4cb33) that the close event "indicates that no more events will be emitted".

hey @nodejs/tsc, do you think a commit that fixes that is semver-major? It's potentially breaking (see code), however it might be worthwhile landing in v14.

mcollina

lgtm

jasnell · 2020-04-27T15:33:33Z

Hmmm hard to say really. This change looks good to me, however.

Readable stream could emendd' after 'close'.

ronag · 2020-04-27T15:53:05Z

rebased to fix conflict

nodejs-github-bot · 2020-04-27T15:53:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31013/ (:heavy_check_mark:)

mcollina · 2020-04-27T17:39:10Z

After speaking with @mafintosh, let's go with a minor and see. I would not backport it to v12.

Readable stream could emit 'end' after 'close'. PR-URL: #33076 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

ronag · 2020-04-28T14:01:17Z

Landed in f64c640

Readable stream could emit 'end' after 'close'. PR-URL: #33076 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Notable changes: - deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha) [#32971](#32971) - doc: add juanarbol as collaborator (Juan José Arboleda) [#32906](#32906) - http: doc deprecate abort and improve docs (Robert Nagy) [#32807](#32807) - module: do not warn when accessing `__esModule` of unfinished exports (Anna Henningsen) [#33048](#33048) - n-api: detect deadlocks in thread-safe function (Gabriel Schulhof) [#32860](#32860) - src: deprecate embedder APIs with replacements (Anna Henningsen) [#32858](#32858) - stream: - don't emit end after close (Robert Nagy) [#33076](#33076) - don't wait for close on legacy streams (Robert Nagy) [#33058](#33058) - pipeline should only destroy un-finished streams (Robert Nagy) [#32968](#32968) PR-URL: #33103

Readable stream could emit 'end' after 'close'. PR-URL: #33076 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Notable changes: - deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha) [#32971](#32971) - doc: add juanarbol as collaborator (Juan José Arboleda) [#32906](#32906) - http: doc deprecate abort and improve docs (Robert Nagy) [#32807](#32807) - module: do not warn when accessing `__esModule` of unfinished exports (Anna Henningsen) [#33048](#33048) - n-api: detect deadlocks in thread-safe function (Gabriel Schulhof) [#32860](#32860) - src: deprecate embedder APIs with replacements (Anna Henningsen) [#32858](#32858) - stream: - don't emit end after close (Robert Nagy) [#33076](#33076) - don't wait for close on legacy streams (Robert Nagy) [#33058](#33058) - pipeline should only destroy un-finished streams (Robert Nagy) [#32968](#32968) - vm: add importModuleDynamically option to compileFunction (Gus Caplan) [#32985](#32985) PR-URL: #33103

ronag added stream Issues and PRs related to the stream subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 26, 2020

ronag force-pushed the stream-end-after-close branch from 841f7af to 879ab7c Compare April 26, 2020 09:41

ronag marked this pull request as ready for review April 26, 2020 09:41

ronag force-pushed the stream-end-after-close branch from 879ab7c to b36eed4 Compare April 26, 2020 09:41

ronag commented Apr 26, 2020

View reviewed changes

ronag force-pushed the stream-end-after-close branch 2 times, most recently from f7b791b to 4ee7287 Compare April 26, 2020 09:45

ronag changed the title ~~stream: don't emit end after closei~~ stream: don't emit end after close Apr 26, 2020

mcollina requested changes Apr 26, 2020

View reviewed changes

mcollina approved these changes Apr 27, 2020

View reviewed changes

jasnell approved these changes Apr 27, 2020

View reviewed changes

stream: don't emit end after close

412129c

Readable stream could emendd' after 'close'.

ronag force-pushed the stream-end-after-close branch from 4ee7287 to 412129c Compare April 27, 2020 15:53

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2020

mcollina removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 27, 2020

mcollina added dont-land-on-v10.x semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 27, 2020

addaleax approved these changes Apr 27, 2020

View reviewed changes

ronag closed this Apr 28, 2020

BethGriggs mentioned this pull request Apr 28, 2020

v14.1.0 proposal #33103

Merged

nicolo-ribaudo mentioned this pull request Apr 30, 2020

[Regression in 14.1.0 - Windows] stdout is sometimes empty #33166

Closed

ronag mentioned this pull request Jul 21, 2020

Unable to export component with .ttf files teambit/bit#2844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: don't emit end after close #33076

stream: don't emit end after close #33076

ronag commented Apr 26, 2020 •

edited

Loading

ronag Apr 26, 2020

ronag Apr 26, 2020

ronag commented Apr 26, 2020

nodejs-github-bot commented Apr 26, 2020

mcollina left a comment

ronag commented Apr 26, 2020

mcollina commented Apr 26, 2020

vweevers commented Apr 26, 2020

mcollina commented Apr 26, 2020

ronag commented Apr 27, 2020 •

edited

Loading

nodejs-github-bot commented Apr 27, 2020

mcollina commented Apr 27, 2020

mcollina left a comment

jasnell commented Apr 27, 2020

ronag commented Apr 27, 2020

nodejs-github-bot commented Apr 27, 2020 •

edited by addaleax

Loading

mcollina commented Apr 27, 2020

ronag commented Apr 28, 2020

stream: don't emit end after close #33076

stream: don't emit end after close #33076

Conversation

ronag commented Apr 26, 2020 • edited Loading

Checklist

ronag Apr 26, 2020

Choose a reason for hiding this comment

ronag Apr 26, 2020

Choose a reason for hiding this comment

ronag commented Apr 26, 2020

nodejs-github-bot commented Apr 26, 2020

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Apr 26, 2020

mcollina commented Apr 26, 2020

vweevers commented Apr 26, 2020

mcollina commented Apr 26, 2020

ronag commented Apr 27, 2020 • edited Loading

nodejs-github-bot commented Apr 27, 2020

mcollina commented Apr 27, 2020

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented Apr 27, 2020

ronag commented Apr 27, 2020

nodejs-github-bot commented Apr 27, 2020 • edited by addaleax Loading

mcollina commented Apr 27, 2020

ronag commented Apr 28, 2020

ronag commented Apr 26, 2020 •

edited

Loading

ronag commented Apr 27, 2020 •

edited

Loading

nodejs-github-bot commented Apr 27, 2020 •

edited by addaleax

Loading