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: use state.ending to see if stream called end() #18170

Conversation

MoonBall
Copy link
Member

@MoonBall MoonBall commented Jan 16, 2018

Calling writable.end() will probably synchronously call
writable.write(), in such a situation the state.ended
is false and writable.write() doesn't trigger writeAfterEnd().

There is an example as below:

const { Writable } = require('stream');

let data = '';
const w = new Writable({
  write: function(chunk, encoding, cb) {
    process.nextTick(() => {
      data += chunk;
      cb();
    });
  },
});

w.on('prefinish', () => {
  // w.write('prefinish');
});
w.on('finish', () => {
  w.write('finish');
});

w.end(() => {
  console.log(data);
});

when 'prefinish' listener doesn't call w.write(), w.end() will synchronously emit 'finish' event. The console's output is: finish.
when 'prefinish' listener calls w.write(), the 'finish' event will be asynchronously emitted. The w.write() that called by the 'finish' listener will trigger writeAfterEnd() and throw a Error.

I think using state.ended to decide whether to trigger writeAfterEnd is diffcult to understand and is somewhat unreasonable.

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

stream

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 16, 2018
@mcollina
Copy link
Member

I do not understand what is the current behavior and what you want to achieve with this change. Can you please edit the description of this PR to clarify that? Examples works better than a textual form.

Can you please add/change the unit tests?

@MoonBall
Copy link
Member Author

@mcollina I added test cases. are they right?

@mcollina
Copy link
Member

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 17, 2018
@mafintosh
Copy link
Member

This is how i would expect things to work today and I would consider this a bug

@mcollina mcollina removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 17, 2018
@MoonBall
Copy link
Member Author

@mcollina There are some failed checks. What should I do?

@mcollina
Copy link
Member

I think it's CI that is flaky.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1208/

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I would feel okay with considering this semver-major just to be careful (but also okay with not doing so).

@mcollina
Copy link
Member

We can land it semver-minor and taking it some more time before backporting.

@mcollina
Copy link
Member

@mcollina
Copy link
Member

Can you please rebase on top of master?

Calling `writable.end()` will probably synchronously call
`writable.write()`, in such a situation the `state.ended`
is false and `writable.write()` doesn't trigger `writeAfterEnd()`.
@MoonBall MoonBall force-pushed the use-state.ending-to-trigger-writeAfterEnd branch from 1401476 to e6d3654 Compare January 18, 2018 17:59
@MoonBall
Copy link
Member Author

Rebased.

@mcollina
Copy link
Member

@mcollina mcollina added semver-minor PRs that contain new features and should be released in the next minor version. baking-for-lts PRs that need to wait before landing in a LTS release. dont-land-on-v4.x labels Jan 19, 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

@mcollina
Copy link
Member

CITGM seems similar, landing.

@mcollina
Copy link
Member

Landed as c7ca07a

@mcollina mcollina closed this Jan 30, 2018
mcollina pushed a commit that referenced this pull request Jan 30, 2018
Calling `writable.end()` will probably synchronously call
`writable.write()`, in such a situation the `state.ended`
is false and `writable.write()` doesn't trigger `writeAfterEnd()`.

PR-URL: #18170
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
Calling `writable.end()` will probably synchronously call
`writable.write()`, in such a situation the `state.ended`
is false and `writable.write()` doesn't trigger `writeAfterEnd()`.

PR-URL: #18170
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request Jan 30, 2018
Notable changes:

* cluster
  - add cwd to cluster.settings (cjihrig) [#18399](#18399)
* deps
  - upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260)
* meta
  - add Leko to collaborators (Leko) [#18117](#18117)
  - add vdeturckheim as collaborator (vdeturckheim) [#18432](#18432)
* n-api
  - expose n-api version in process.versions (Michael Dawson) [#18067](#18067)
* perf_hooks
  - add performance.clear() (James M Snell) [#18046](#18046)
* stream
  - avoid writeAfterEnd() while ending (陈刚) [#18170](#18170)

PR-URL: #18464
evanlucas added a commit that referenced this pull request Jan 31, 2018
Notable changes:

* cluster
  - add cwd to cluster.settings (cjihrig) [#18399](#18399)
* deps
  - upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260)
* meta
  - add Leko to collaborators (Leko) [#18117](#18117)
  - add vdeturckheim as collaborator (vdeturckheim) [#18432](#18432)
* n-api
  - expose n-api version in process.versions (Michael Dawson) [#18067](#18067)
* perf_hooks
  - add performance.clear() (James M Snell) [#18046](#18046)
* stream
  - avoid writeAfterEnd() while ending (陈刚) [#18170](#18170)

PR-URL: #18464
evanlucas added a commit that referenced this pull request Feb 1, 2018
Notable changes:

* cluster
  - add cwd to cluster.settings (cjihrig) [#18399](#18399)
* deps
  - upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260)
* meta
  - add Leko to collaborators (Leko) [#18117](#18117)
  - add vdeturckheim as collaborator (vdeturckheim) [#18432](#18432)
* n-api
  - expose n-api version in process.versions (Michael Dawson) [#18067](#18067)
* perf_hooks
  - add performance.clear() (James M Snell) [#18046](#18046)
* stream
  - avoid writeAfterEnd() while ending (陈刚) [#18170](#18170)

PR-URL: #18464
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Calling `writable.end()` will probably synchronously call
`writable.write()`, in such a situation the `state.ended`
is false and `writable.write()` doesn't trigger `writeAfterEnd()`.

PR-URL: nodejs#18170
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Notable changes:

* cluster
  - add cwd to cluster.settings (cjihrig) [nodejs#18399](nodejs#18399)
* deps
  - upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260)
* meta
  - add Leko to collaborators (Leko) [nodejs#18117](nodejs#18117)
  - add vdeturckheim as collaborator (vdeturckheim) [nodejs#18432](nodejs#18432)
* n-api
  - expose n-api version in process.versions (Michael Dawson) [nodejs#18067](nodejs#18067)
* perf_hooks
  - add performance.clear() (James M Snell) [nodejs#18046](nodejs#18046)
* stream
  - avoid writeAfterEnd() while ending (陈刚) [nodejs#18170](nodejs#18170)

PR-URL: nodejs#18464
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants