-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
stdio: make stdout and stderr emit 'close' on destroy #26691
Changes from all commits
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 |
---|---|---|
|
@@ -2,7 +2,26 @@ | |
|
||
exports.getMainThreadStdio = getMainThreadStdio; | ||
|
||
function dummyDestroy(err, cb) { cb(err); } | ||
function dummyDestroy(err, cb) { | ||
// SyncWriteStream does not use the stream | ||
// destroy mechanism for some legacy reason. | ||
// TODO(mcollina): remove when | ||
// https://github.com/nodejs/node/pull/26690 lands. | ||
if (typeof cb === 'function') { | ||
cb(err); | ||
} | ||
|
||
// We need to emit 'close' anyway so that the closing | ||
// of the stream is observable. We just make sure we | ||
// are not going to do it twice. | ||
// The 'close' event is needed so that finished and | ||
// pipeline work correctly. | ||
if (!this._writableState.emitClose) { | ||
process.nextTick(() => { | ||
this.emit('close'); | ||
}); | ||
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. How come the 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. The previous cb() could emitting an error via nextTick. Close should be emitted afterwards. 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. Are you ok/can this PR land, or would you like to see some different resolution for this? |
||
} | ||
} | ||
|
||
function getMainThreadStdio() { | ||
var stdin; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
const { Transform, Readable, pipeline } = require('stream'); | ||
const assert = require('assert'); | ||
|
||
const reader = new Readable({ | ||
read(size) { this.push('foo'); } | ||
}); | ||
|
||
let count = 0; | ||
|
||
const err = new Error('this-error-gets-hidden'); | ||
|
||
const transform = new Transform({ | ||
transform(chunk, enc, cb) { | ||
if (count++ >= 5) | ||
this.emit('error', err); | ||
else | ||
cb(null, count.toString() + '\n'); | ||
} | ||
}); | ||
|
||
pipeline( | ||
reader, | ||
transform, | ||
process.stdout, | ||
common.mustCall((e) => { | ||
assert.strictEqual(e, err); | ||
}) | ||
); |
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.
What exactly are you intending to remove? The
dummyDestroy()
function? It’s intentional that stdio streams, SyncWriteStream or not, cannot “really” be closed (so that file descriptors 0, 1, 2 don’t accidentally get closed).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.
The if check underneath. See the linked PR. Basically we can have a callback or not for this function.
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.
#26690 was merged so this needs to go now?
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! However that was a semver-major commit and this is not. I would prefer to issue a follow-up PR that will not be backported. This can (and should) be backported.