-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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: prevent 'end' to be emitted after 'error' #20104
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,10 +8,14 @@ function destroy(err, cb) { | |
this._writableState.destroyed; | ||
|
||
if (readableDestroyed || writableDestroyed) { | ||
const readableErrored = this._readableState && | ||
this._readableState.errorEmitted; | ||
const writableErrored = this._writableState && | ||
this._writableState.errorEmitted; | ||
|
||
if (cb) { | ||
cb(err); | ||
} else if (err && | ||
(!this._writableState || !this._writableState.errorEmitted)) { | ||
} else if (err && !(readableErrored || writableErrored)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: maybe it's just me but I find it a lot easier to read if De Morgan is applied. |
||
process.nextTick(emitErrorNT, this, err); | ||
} | ||
return this; | ||
|
@@ -32,6 +36,11 @@ function destroy(err, cb) { | |
this._destroy(err || null, (err) => { | ||
if (!cb && err) { | ||
process.nextTick(emitErrorAndCloseNT, this, err); | ||
|
||
if (this._readableState) { | ||
this._readableState.errorEmitted = true; | ||
} | ||
|
||
if (this._writableState) { | ||
this._writableState.errorEmitted = true; | ||
} | ||
|
@@ -65,6 +74,7 @@ function undestroy() { | |
this._readableState.reading = false; | ||
this._readableState.ended = false; | ||
this._readableState.endEmitted = false; | ||
this._readableState.errorEmitted = false; | ||
} | ||
|
||
if (this._writableState) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ if (process.argv[2] === 'child') { | |
assert.strictEqual(signal, null); | ||
})); | ||
|
||
child.stderr.pipe(process.stderr); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? |
||
child.stdout.destroy(); | ||
child.send('go'); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
const { Duplex } = require('stream'); | ||
const { strictEqual } = require('assert'); | ||
|
||
const duplex = new Duplex({ | ||
write(chunk, enc, cb) { | ||
cb(new Error('kaboom')); | ||
}, | ||
read() { | ||
this.push(null); | ||
} | ||
}); | ||
|
||
duplex.on('error', common.mustCall(function() { | ||
strictEqual(this._readableState.errorEmitted, true); | ||
strictEqual(this._writableState.errorEmitted, true); | ||
})); | ||
|
||
duplex.on('end', common.mustNotCall()); | ||
|
||
duplex.end('hello'); | ||
duplex.on('data', function() {}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked in enough detail but I guess there's no way to do this in
_stream_duplex.js
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal would be to change this into
stream.destroy(err)
. But that's a more massive change, see #20096.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can't we only use one of these and put it outside the wrapper
if
instead of duplicating it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't as the two branches are significantly different. However we can move it in its own function. Should I do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm why, in this branch there are only two next tick calls so it's not a problem, in the
else
below there is onlycb(er);
before but I think it's not necessary to call the callback before setting the flag. I mean something like this:but I didn't test it. If it doesn't work or if I am missing something, ignore me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
stream._writableState.errorEmitted = true
can stay as is if the user provided callback needs to find it set tofalse
(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.