stream: emit .write() end error in next tick #4749

phillipj · 2016-01-18T20:22:25Z

Next up in my TODO triaging, this time in _stream_writable.js.

Assuming it should be labelled semver-major as it changes from emitting error synchronously to async.

evanlucas · 2016-01-18T23:31:03Z

lib/_stream_writable.js

-  process.nextTick(cb, er);
+function writeAfterEndErr(stream, cb) {
+  const err = new Error('write after end');
+  process.nextTick(() => stream.emit('error', err));


It would probably be better to just call process.nextTick once here and both emit the error and call the callback.

It would probably be better to just call process.nextTick once here and both emit the error and call the callback.

Sure thing, agreed it looked strange. Just pushed an update.

jasnell · 2016-01-19T01:23:06Z

Generally LGTM once @evanlucas, @mscdex and CI are happy :-)

evanlucas · 2016-01-19T01:49:09Z

Marking as semver-major as it is a behavior change

Fishrock123 · 2016-01-19T15:48:55Z

@nodejs/streams

mafintosh · 2016-01-19T17:22:03Z

lib/_stream_writable.js

+  process.nextTick(() => {
+    stream.emit('error', err);
+    cb(err);
+  });


i'm not sure we transform arrow functions yet in the readable-stream builds. @calvinmetcalf probably knows more about this?

i'm not sure we transform arrow functions yet in the readable-stream builds.

I'll happily change it to an ordinary function. Really curious about what kind of transformation you're talking about though?

the readable-stream module is build from the node source using a couple of build methods, https://github.com/nodejs/readable-stream/tree/master/build to get better browser support.

we do not support arrow functions at the moment, and it would be tricky to do so in a performant manner, that being said, you could change this code to be,

function writeAfterEnd(stream, cb) { const err = new Error('write after end'); stream.emit('error', err); cb(err); }

and then replace the use of writeAfterEndErr(this, cb) with

process.nextTick(writeAfterEnd, this, cb);

and this would avoid creating an unnecessary closure

the readable-stream module is build from the node source using a couple of build methods, https://github.com/nodejs/readable-stream/tree/master/build to get better browser support.

Neat, that's some nice wizardry.

and then replace the use of writeAfterEndErr(this, cb) with

Thanks @calvinmetcalf, just pushed an update 👍

chrisdickinson · 2016-01-19T20:55:49Z

I worry about this a little, in that streams can be enormously fussy when it comes to when things happen. A CITGM run with this would ease my mind a bit (especially if CITGM contains stream-using packages, like gulp.)

MylesBorins · 2016-01-19T20:58:46Z

@chrisdickinson can you make a short list of stream based packages you would like to see added to citgm?

I'd be more than happy to add them then run the smoke-testing suite

MylesBorins · 2016-01-19T20:59:02Z

current lookup for reference --> https://github.com/nodejs/citgm/blob/master/lib/lookup.json

This changes the behaviour of error event emitted when writing to a stream after it has ended, from synchronously to asynchronously. PR-URL: nodejs#4749

phillipj · 2016-01-21T21:30:15Z

I just created these citgm reports @chrisdickinson:

This PR

Flaky Modules

express
- version 4.13.3
- Error: The canary is dead.
react
- version 0.14.6
- Error: Install Failed

v4.2.5

Flaky Modules

express
- version 4.13.3
- Error: The canary is dead.

v5.5.0

Flaky Modules

express
- version 4.13.3
- Error: The canary is dead.
socket.io
- version 1.4.4
- Error: The canary is dead.
react
- version 0.14.6
- Error: Install Failed

P.S. react fails deliberately as it requires v4.x obviously.

jasnell · 2016-01-23T01:57:19Z

Unfortunately I think failures on those in citgm are common. @thealphanerd can say for sure tho

MylesBorins · 2016-01-23T02:01:22Z

Those are all expected 😄

MylesBorins · 2016-01-23T02:02:06Z

fwiw I think @chrisdickinson wanted to add more modules to citgm that were focused on streams.

chrisdickinson · 2016-01-23T04:11:46Z

I might see if @phated can recommend some modules from the Gulp ecosystem.

phated · 2016-01-25T20:07:26Z

@chrisdickinson what are you looking for?

jasnell · 2016-01-27T21:06:47Z

@phated ... we'd like to get a handful of stream-related modules to add to our smoke-testing infrastructure (http://github.com/nodejs/citgm). We run the tests for every new release and to get an idea of whether a particular change will break the ecosystem.

phated · 2016-01-31T22:16:31Z

vinyl, vinyl-fs, probably something like gulp-jade or other popular plugins (I don't have actual numbers on any of that)

The most important one is probably vinyl-fs which will be receiving even more tests coming up very soon.

jasnell · 2016-02-01T17:22:28Z

@phated... excellent. /cc @thealphanerd

MylesBorins · 2016-02-02T23:42:56Z

I've opened a PR adding vinyl, vinyl-fs, and readable-stream to citgm.

I'm open to other suggestions

nodejs/citgm#68

phillipj · 2016-02-21T22:21:55Z

Updated results after mentioned updates to citgm ^^

Flaky Modules

socket.io
- version 1.4.5
- Error: The canary is dead.
react
- version 0.14.7
- Error: Install Failed
spdy
- version 3.2.0
- Error: The canary is dead.

Identical results ran on master and this branch/PR, in other words doesn't seem like this PR introduces more failures..

@thealphanerd is the spdy failure expected?

jasnell · 2016-03-02T16:48:26Z

FWIW, still LGTM /cc @nodejs/ctc

jasnell · 2016-03-02T16:50:13Z

Actually.. looks like this change is also applied in #5251 ... may be able to close this in favor of that one

phillipj · 2016-03-02T19:15:02Z

Yep, surely looks like we've done some duplicate work on this one. As #5251 fixes alot more of these issues, I'll close this one and re-open if the other one doesn't land.

silverwind added the stream Issues and PRs related to the stream subsystem. label Jan 18, 2016

evanlucas reviewed Jan 18, 2016
View reviewed changes

evanlucas added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 19, 2016

phillipj force-pushed the emit-ws-end-error-nt branch from b732435 to 227c9f5 Compare January 19, 2016 15:51

phillipj force-pushed the emit-ws-end-error-nt branch from 227c9f5 to 76f6165 Compare January 19, 2016 15:52

mafintosh reviewed Jan 19, 2016
View reviewed changes

stream: emit .write() end error in next tick.

1899689

This changes the behaviour of error event emitted when writing to a stream after it has ended, from synchronously to asynchronously. PR-URL: nodejs#4749

phillipj force-pushed the emit-ws-end-error-nt branch from 76f6165 to 1899689 Compare January 21, 2016 19:47

jasnell added the error-handling label Mar 2, 2016

phillipj closed this Mar 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: emit .write() end error in next tick #4749

stream: emit .write() end error in next tick #4749

phillipj commented Jan 18, 2016

evanlucas Jan 18, 2016

mscdex Jan 19, 2016

phillipj Jan 19, 2016

jasnell commented Jan 19, 2016

evanlucas commented Jan 19, 2016

Fishrock123 commented Jan 19, 2016

mafintosh Jan 19, 2016

phillipj Jan 21, 2016

mafintosh Jan 21, 2016

calvinmetcalf Jan 21, 2016

phillipj Jan 21, 2016

chrisdickinson commented Jan 19, 2016

MylesBorins commented Jan 19, 2016

MylesBorins commented Jan 19, 2016

phillipj commented Jan 21, 2016

jasnell commented Jan 23, 2016

MylesBorins commented Jan 23, 2016

MylesBorins commented Jan 23, 2016

chrisdickinson commented Jan 23, 2016

phated commented Jan 25, 2016

jasnell commented Jan 27, 2016

phated commented Jan 31, 2016

jasnell commented Feb 1, 2016

MylesBorins commented Feb 2, 2016

phillipj commented Feb 21, 2016

jasnell commented Mar 2, 2016

jasnell commented Mar 2, 2016

phillipj commented Mar 2, 2016

stream: emit .write() end error in next tick #4749

stream: emit .write() end error in next tick #4749

Conversation

phillipj commented Jan 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jan 19, 2016

evanlucas commented Jan 19, 2016

Fishrock123 commented Jan 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdickinson commented Jan 19, 2016

MylesBorins commented Jan 19, 2016

MylesBorins commented Jan 19, 2016

phillipj commented Jan 21, 2016

This PR

Flaky Modules

v4.2.5

Flaky Modules

v5.5.0

Flaky Modules

jasnell commented Jan 23, 2016

MylesBorins commented Jan 23, 2016

MylesBorins commented Jan 23, 2016

chrisdickinson commented Jan 23, 2016

phated commented Jan 25, 2016

jasnell commented Jan 27, 2016

phated commented Jan 31, 2016

jasnell commented Feb 1, 2016

MylesBorins commented Feb 2, 2016

phillipj commented Feb 21, 2016

Flaky Modules

jasnell commented Mar 2, 2016

jasnell commented Mar 2, 2016

phillipj commented Mar 2, 2016