iframe-proxy

MoonBall · 2018-01-16T06:47:57Z

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

mcollina · 2018-01-17T11:59:53Z

MoonBall · 2018-01-17T16:22:23Z

@mcollina I added test cases. are they right?

mcollina · 2018-01-17T16:29:39Z

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

mafintosh · 2018-01-17T17:13:23Z

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

MoonBall · 2018-01-17T17:39:20Z

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

mcollina · 2018-01-17T22:19:10Z

I think it's CI that is flaky.

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

addaleax

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

mcollina · 2018-01-17T22:33:33Z

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

mcollina · 2018-01-18T16:33:46Z

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

mcollina · 2018-01-18T16:39:04Z

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 · 2018-01-18T18:00:51Z

Rebased.

mcollina · 2018-01-19T11:08:55Z

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

mcollina · 2018-01-29T15:08:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12801/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1241/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1242/

mcollina

LGTM

mcollina · 2018-01-30T14:46:22Z

CITGM seems similar, landing.

mcollina · 2018-01-30T15:08:33Z

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>

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

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>

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

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 16, 2018

apapirovski requested a review from mcollina January 16, 2018 16:37

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

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

addaleax approved these changes Jan 17, 2018

View reviewed changes

stream: use state.ending to see if stream called end()

e6d3654

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 force-pushed the use-state.ending-to-trigger-writeAfterEnd branch from 1401476 to e6d3654 Compare January 18, 2018 17:59

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

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

mcollina approved these changes Jan 29, 2018

View reviewed changes

jasnell approved these changes Jan 29, 2018

View reviewed changes

mcollina closed this Jan 30, 2018

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

MylesBorins added the dont-land-on-v8.x label Aug 2, 2018

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Uh oh!

Conversation

MoonBall commented Jan 16, 2018 • edited by mcollina Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mcollina commented Jan 17, 2018

Uh oh!

MoonBall commented Jan 17, 2018

Uh oh!

mcollina commented Jan 17, 2018

Uh oh!

mafintosh commented Jan 17, 2018

Uh oh!

MoonBall commented Jan 17, 2018

Uh oh!

mcollina commented Jan 17, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jan 17, 2018

Uh oh!

mcollina commented Jan 18, 2018

Uh oh!

mcollina commented Jan 18, 2018

Uh oh!

MoonBall commented Jan 18, 2018

Uh oh!

mcollina commented Jan 19, 2018

Uh oh!

mcollina commented Jan 29, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jan 30, 2018

Uh oh!

mcollina commented Jan 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

MoonBall commented Jan 16, 2018 •

edited by mcollina

Loading