stream: prevent 'end' to be emitted after 'error' #20104

mcollina · 2018-04-17T15:30:28Z

This PR adds _readableState.errorEmitted and add the tracking of it.

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

mcollina · 2018-04-17T15:32:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14353/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1371

cc @nodejs/streams @mafintosh

mcollina · 2018-04-17T15:32:47Z

cc @davepacheco

apapirovski

LGTM but want to give it another look before full on approval.

apapirovski · 2018-04-17T15:32:40Z

lib/_stream_readable.js

@@ -99,6 +99,9 @@ function ReadableState(options, stream, isDuplex) {
  this.endEmitted = false;
  this.reading = false;

+  // flipped if an 'error' is emitted


Nit: capitalize & end with a dot. (And yes, I know it totally doesn't match the rest of this file but it is our new style guide...)

apapirovski · 2018-04-17T15:34:55Z

lib/_stream_writable.js

@@ -424,12 +424,22 @@ function onwriteError(stream, state, sync, er, cb) {
    // this can emit finish, and it will always happen
    // after error
    process.nextTick(finishMaybe, stream, state);
+
+    // needed for duplex, fixes https://github.com/nodejs/node/issues/6083
+    if (stream._readableState) {


I haven't looked in enough detail but I guess there's no way to do this in _stream_duplex.js instead?

My goal would be to change this into stream.destroy(err). But that's a more massive change, see #20096.

Nit: can't we only use one of these and put it outside the wrapper if instead of duplicating it?

we can't as the two branches are significantly different. However we can move it in its own function. Should I do that?

Hmm why, in this branch there are only two next tick calls so it's not a problem, in the else below there is only cb(er); before but I think it's not necessary to call the callback before setting the flag. I mean something like this:

diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index d21daf0541..04b344bc26 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -417,6 +417,13 @@ function doWrite(stream, state, writev, len, chunk, encoding, cb) { function onwriteError(stream, state, sync, er, cb) { --state.pendingcb; + // needed for duplex, fixes https://github.com/nodejs/node/issues/6083 + if (stream._readableState) { + stream._readableState.errorEmitted = true; + } + + stream._writableState.errorEmitted = true; + if (sync) { // defer the callback if we are being called synchronously // to avoid piling up things on the stack @@ -424,13 +431,11 @@ function onwriteError(stream, state, sync, er, cb) { // this can emit finish, and it will always happen // after error process.nextTick(finishMaybe, stream, state); - stream._writableState.errorEmitted = true; stream.emit('error', er); } else { // the caller expect this to happen before if // it is async cb(er); - stream._writableState.errorEmitted = true; stream.emit('error', er); // this can emit finish, but finish must // always follow error

but I didn't test it. If it doesn't work or if I am missing something, ignore me.

In the second block, you are setting errorEmitted before calling the callback. I remember spending a significant amount of time on this block of code, and these two paths should remain separated. It's something I'd prefer not to refactor in this PR.

Yes stream._writableState.errorEmitted = true can stay as is if the user provided callback needs to find it set to false (why?) but I see no reason to not move the other part. There can't be existing code that relies on new code.

Anyway it's a just a nit and it can be ignored.

apapirovski · 2018-04-17T15:36:43Z

lib/internal/streams/destroy.js

@@ -6,12 +6,15 @@ function destroy(err, cb) {
    this._readableState.destroyed;
  const writableDestroyed = this._writableState &&
    this._writableState.destroyed;
+  const readableErrored = this._readableState &&


Nit: can we create these inside the if block just below? It doesn't look like they're used outside of it.

apapirovski · 2018-04-17T15:45:14Z

We should probably at least let this bake before LTS if not outright not land it. The fact that we even have tests that rely on this behaviour is concerning...

mcollina · 2018-04-17T15:56:53Z

We should probably at least let this bake before LTS if not outright not land it.

This should definitely bake for a bit before going into LTS.

This PR adds _readableState.errorEmitted and add the tracking of it. Fixes: nodejs#6083

lpinca · 2018-04-17T19:43:29Z

test/parallel/test-stream-duplex-error-write.js

+duplex.on('end', common.mustNotCall());
+
+duplex.end('hello');
+duplex.on('data', function() {});


Nit: duplex.resume() is cleaner.

lpinca · 2018-04-17T20:07:47Z

Ops I commented on the fork.

lpinca · 2018-04-17T20:09:15Z

lib/internal/streams/destroy.js

    if (cb) {
      cb(err);
-    } else if (err &&
-               (!this._writableState || !this._writableState.errorEmitted)) {
+    } else if (err && !(readableErrored || writableErrored)) {


Nit: maybe it's just me but I find it a lot easier to read if De Morgan is applied.

lpinca · 2018-04-17T20:09:43Z

test/parallel/test-process-external-stdio-close.js

@@ -21,6 +21,7 @@ if (process.argv[2] === 'child') {
    assert.strictEqual(signal, null);
  }));

+  child.stderr.pipe(process.stderr);


Why is this needed?

mcollina · 2018-04-17T22:40:20Z

CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1372/

mcollina · 2018-04-17T22:41:17Z

CI (only the ones that failed): https://ci.nodejs.org/job/node-test-commit-linux/18088/

BridgeAR · 2018-04-18T02:13:21Z

What is this semver wise? This should be semver-major, right?

lpinca · 2018-04-18T05:20:12Z

@BridgeAR yes, this is semver-major imho.

mcollina · 2018-04-18T07:12:30Z

This solves an extremely bad bug that has been open for a long period of time. errorEmitted has been part of Writable for a long period of time, but the same logic was never added to Readable.

The underlining contract of streams is that after 'error' is emitted you cannot trust any other event that is being emitted. Ideally, no stream event should be emitted after 'error' is emitted.

This relies on the .destroy() that has been added on 8. In fact, it should have been part of it. I preferred not to add this logic then because I couldn't find a failing scenario, which #6083 provided.

I'm certain that this could lend on 10 as a minor or patch, due to the new error semantics that @mafintosh has added in #19836. In fact, it should be in as a way to strengthen that invariant.

As of backporting to 8, this is possible, as the current behavior is not deterministic (the test case in #6083 fails 50% of the time).

@mafintosh, what do you think?

jasnell · 2018-04-19T20:22:04Z

Still LGTM

mafintosh · 2018-04-20T10:25:03Z

LGTM from me. I'm noting that in my userland stream modules we already consider the first error to be "terminal", and ignore any following events because you can't rely on their behaivor after an error. Good job @mcollina

This is a semver-minor, as it's a bug fix.

I would only attempt a backport if it is easy to do so (you know the complexity of this more than me @mcollina), as the userland modules handles these cases right now either way.

mcollina · 2018-04-20T10:27:48Z

landing as a semver-minor. It would be fantastic if this could go into Node 10 @jasnell. Feel free to remove if from the milestone if it's over the finishing line.

lpinca · 2018-04-20T10:28:57Z

If it's a bug fix why semver-minor, it doesn't add anything that should be publicly accessible.

mcollina · 2018-04-20T10:30:05Z

removed semver-minor.

mcollina · 2018-04-20T10:30:55Z

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

mcollina · 2018-04-20T13:12:58Z

Landed in 0857790

This PR adds _readableState.errorEmitted and add the tracking of it. Fixes: #6083 PR-URL: #20104 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

mscdex · 2018-04-25T22:18:25Z

If we are to keep this change, I think this should be marked semver-major as it is a change in behavior and can break existing code (I stumbled upon this myself when trying to debug with node v10). There is not even any indication that I could find in the documentation that this is the intended behavior. Otherwise, we should revert this IMO because it didn't even have time to land in a v9 release and was merged only 5 days ago.

/cc @nodejs/collaborators

mcollina requested review from mafintosh, jasnell and trivikr April 17, 2018 15:30

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

mcollina mentioned this pull request Apr 17, 2018

TCP socket emitted 'end' event after 'error' event #6083

Closed

apapirovski reviewed Apr 17, 2018

View reviewed changes

mcollina added dont-land-on-v4.x labels Apr 17, 2018

jasnell approved these changes Apr 17, 2018

View reviewed changes

stream: prevent 'end' to be emitted after 'error'

5b69058

This PR adds _readableState.errorEmitted and add the tracking of it. Fixes: nodejs#6083

mcollina force-pushed the add-readableState-errorEmitted branch from 9b0fd56 to 5b69058 Compare April 17, 2018 15:58

lpinca reviewed Apr 17, 2018

View reviewed changes

lpinca approved these changes Apr 17, 2018

View reviewed changes

fixup: nits

4038163

BridgeAR approved these changes Apr 18, 2018

View reviewed changes

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

mcollina added the baking-for-lts PRs that need to wait before landing in a LTS release. label Apr 18, 2018

trivikr approved these changes Apr 18, 2018

View reviewed changes

mcollina requested a review from a team April 18, 2018 18:12

mafintosh approved these changes Apr 20, 2018

View reviewed changes

mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 20, 2018

mcollina added this to the 10.0.0 milestone Apr 20, 2018

mcollina removed the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 20, 2018

mcollina closed this Apr 20, 2018

mcollina deleted the add-readableState-errorEmitted branch April 20, 2018 13:13

This was referenced Apr 26, 2018

stream: 'end' is no longer emitted after 'error' #20334

Closed

test: fix failing unit tests due to assert in exit event handler mscdex/ssh2-streams#104

Closed

mscdex added dont-land-on-v8.x labels May 1, 2018

addaleax removed the baking-for-lts PRs that need to wait before landing in a LTS release. label May 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: prevent 'end' to be emitted after 'error' #20104

stream: prevent 'end' to be emitted after 'error' #20104

mcollina commented Apr 17, 2018

mcollina commented Apr 17, 2018

mcollina commented Apr 17, 2018

apapirovski left a comment

apapirovski Apr 17, 2018

apapirovski Apr 17, 2018

mcollina Apr 17, 2018

lpinca Apr 17, 2018 •

edited

Loading

mcollina Apr 17, 2018

lpinca Apr 18, 2018

mcollina Apr 18, 2018

lpinca Apr 18, 2018

apapirovski Apr 17, 2018

apapirovski commented Apr 17, 2018

mcollina commented Apr 17, 2018

lpinca Apr 17, 2018

lpinca commented Apr 17, 2018

lpinca Apr 17, 2018

lpinca Apr 17, 2018

mcollina commented Apr 17, 2018

mcollina commented Apr 17, 2018

BridgeAR commented Apr 18, 2018

lpinca commented Apr 18, 2018

mcollina commented Apr 18, 2018

jasnell commented Apr 19, 2018

mafintosh commented Apr 20, 2018

mcollina commented Apr 20, 2018

lpinca commented Apr 20, 2018

mcollina commented Apr 20, 2018

mcollina commented Apr 20, 2018

mcollina commented Apr 20, 2018

mscdex commented Apr 25, 2018 •

edited

Loading

stream: prevent 'end' to be emitted after 'error' #20104

stream: prevent 'end' to be emitted after 'error' #20104

Conversation

mcollina commented Apr 17, 2018

Checklist

mcollina commented Apr 17, 2018

mcollina commented Apr 17, 2018

apapirovski left a 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

lpinca Apr 17, 2018 • edited Loading

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

Choose a reason for hiding this comment

apapirovski commented Apr 17, 2018

mcollina commented Apr 17, 2018

Choose a reason for hiding this comment

lpinca commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Apr 17, 2018

mcollina commented Apr 17, 2018

BridgeAR commented Apr 18, 2018

lpinca commented Apr 18, 2018

mcollina commented Apr 18, 2018

jasnell commented Apr 19, 2018

mafintosh commented Apr 20, 2018

mcollina commented Apr 20, 2018

lpinca commented Apr 20, 2018

mcollina commented Apr 20, 2018

mcollina commented Apr 20, 2018

mcollina commented Apr 20, 2018

mscdex commented Apr 25, 2018 • edited Loading

lpinca Apr 17, 2018 •

edited

Loading

mscdex commented Apr 25, 2018 •

edited

Loading