Skip to content

console,process: refactoring stdout/stderr error handling #6174

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

console,process: refactoring stdout/stderr error handling #6174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
94fc9b5
process: use proper getter property syntax
jasnell Apr 13, 2016
7916d60
process: on error in stdout/stderr, reopen as null
jasnell Apr 13, 2016
6d347ec
test: move known-issues/test-process-external-stdio-close to parallel
jasnell Apr 13, 2016
2ddd056
console: simplify console impl with ES6 mechanisms
jasnell Apr 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 10 additions & 15 deletions lib/console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,26 +35,25 @@ function Console(stdout, stderr) {
}
}

Console.prototype.log = function() {
this._stdout.write(util.format.apply(null, arguments) + '\n');
Console.prototype.log = function(...args) {
this._stdout.write(`${util.format(...args)}\n`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't ...args still slightly slower than .apply?

Not that it would probably be measurable given log.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW v8 5.x supposedly has improved this significantly (according to the v8 blog), but we will have to test and see how it compares there.

Also, the other day I happened to notice that backtick strings are much slower to create than typical concatenation. I'm not sure what, if any, impact it has on GC though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, as indicated, much of this change is speculative. The code cleanups
make a few assumptions about what v8 v5 should be doing as far as
optimizations but those assumptions have yet to be tested. Those specific
changes can be pulled back out of this if necessary tho.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've separated the more general modifications to console out to a separate PR #6233

};


Console.prototype.info = Console.prototype.log;


Console.prototype.warn = function() {
this._stderr.write(util.format.apply(null, arguments) + '\n');
Console.prototype.warn = function(...args) {
this._stderr.write(`${util.format(...args)}\n`);
};


Console.prototype.error = Console.prototype.warn;


Console.prototype.dir = function(object, options) {
this._stdout.write(util.inspect(object, util._extend({
customInspect: false
}, options)) + '\n');
options = Object.assign({customInspect: false}, options);
this._stdout.write(`${util.inspect(object, options)}\n`);
};


Expand All @@ -75,24 +74,20 @@ Console.prototype.timeEnd = function(label) {
};


Console.prototype.trace = function trace() {
Console.prototype.trace = function trace(...args) {
// TODO probably can to do this better with V8's debug object once that is
// exposed.
var err = new Error();
err.name = 'Trace';
err.message = util.format.apply(null, arguments);
err.message = util.format(...args);
Error.captureStackTrace(err, trace);
this.error(err.stack);
};


Console.prototype.assert = function(expression) {
Console.prototype.assert = function(expression, ...args) {
if (!expression) {
const argsLen = arguments.length || 1;
const arr = new Array(argsLen - 1);
for (var i = 1; i < argsLen; i++)
arr[i - 1] = arguments[i];
require('assert').ok(false, util.format.apply(null, arr));
require('assert').ok(false, util.format(...args));
}
};

Expand Down
225 changes: 130 additions & 95 deletions lib/internal/process/stdio.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,110 +5,129 @@ exports.setup = setupStdio;
function setupStdio() {
var stdin, stdout, stderr;

process.__defineGetter__('stdout', function() {
if (stdout) return stdout;
stdout = createWritableStdioStream(1);
stdout.destroy = stdout.destroySoon = function(er) {
er = er || new Error('process.stdout cannot be closed.');
stdout.emit('error', er);
};
if (stdout.isTTY) {
process.on('SIGWINCH', function() {
stdout._refreshSize();
function cannotCloseStderr(er) {
throw new Error('process.stderr cannot be closed.');
}
function cannotCloseStdout(er) {
throw new Error('process.stdout cannot be closed.');
}

Object.defineProperty(process, 'stdout', {
configurable: false,
enumerable: true,
get: () => {
if (stdout) return stdout;
stdout = createWritableStdioStream(1);
stdout.once('error', (err) => {
stdout = createNullStream(1);
stdout.destroy = stdout.destroySoon = cannotCloseStdout;
process.nextTick(() => process.emit('stdio-error', err, 1));
});
stdout.destroy = stdout.destroySoon = cannotCloseStdout;
if (stdout.isTTY) {
process.on('SIGWINCH', () => stdout._refreshSize());
}
return stdout;
}
return stdout;
});

process.__defineGetter__('stderr', function() {
if (stderr) return stderr;
stderr = createWritableStdioStream(2);
stderr.destroy = stderr.destroySoon = function(er) {
er = er || new Error('process.stderr cannot be closed.');
stderr.emit('error', er);
};
if (stderr.isTTY) {
process.on('SIGWINCH', function() {
stderr._refreshSize();
Object.defineProperty(process, 'stderr', {
configurable: false,
enumerable: true,
get: () => {
if (stderr) return stderr;
stderr = createWritableStdioStream(2);
stderr.once('error', (err) => {
stderr = createNullStream(2);
stderr.destroy = stderr.destroySoon = cannotCloseStderr;
process.nextTick(() => process.emit('stdio-error', err, 2));
});
stderr.destroy = stderr.destroySoon = cannotCloseStderr;
if (stderr.isTTY) {
process.on('SIGWINCH', () => stderr._refreshSize());
}
return stderr;
}
return stderr;
});

process.__defineGetter__('stdin', function() {
if (stdin) return stdin;

var tty_wrap = process.binding('tty_wrap');
var fd = 0;

switch (tty_wrap.guessHandleType(fd)) {
case 'TTY':
var tty = require('tty');
stdin = new tty.ReadStream(fd, {
highWaterMark: 0,
readable: true,
writable: false
});
break;

case 'FILE':
var fs = require('fs');
stdin = new fs.ReadStream(null, { fd: fd, autoClose: false });
break;

case 'PIPE':
case 'TCP':
var net = require('net');

// It could be that process has been started with an IPC channel
// sitting on fd=0, in such case the pipe for this fd is already
// present and creating a new one will lead to the assertion failure
// in libuv.
if (process._channel && process._channel.fd === fd) {
stdin = new net.Socket({
handle: process._channel,
readable: true,
writable: false
});
} else {
stdin = new net.Socket({
fd: fd,
Object.defineProperty(process, 'stdin', {
configurable: false,
enumerable: true,
get: () => {
if (stdin) return stdin;

const tty_wrap = process.binding('tty_wrap');
const fd = 0;

switch (tty_wrap.guessHandleType(fd)) {
case 'TTY':
const tty = require('tty');
stdin = new tty.ReadStream(fd, {
highWaterMark: 0,
readable: true,
writable: false
});
}
// Make sure the stdin can't be `.end()`-ed
stdin._writableState.ended = true;
break;

default:
// Probably an error on in uv_guess_handle()
throw new Error('Implement me. Unknown stdin file type!');
}

// For supporting legacy API we put the FD here.
stdin.fd = fd;
break;

case 'FILE':
const fs = require('fs');
stdin = new fs.ReadStream(null, { fd: fd, autoClose: false });
break;

case 'PIPE':
case 'TCP':
const net = require('net');

// It could be that process has been started with an IPC channel
// sitting on fd=0, in such case the pipe for this fd is already
// present and creating a new one will lead to the assertion failure
// in libuv.
if (process._channel && process._channel.fd === fd) {
stdin = new net.Socket({
handle: process._channel,
readable: true,
writable: false
});
} else {
stdin = new net.Socket({
fd: fd,
readable: true,
writable: false
});
}
// Make sure the stdin can't be `.end()`-ed
stdin._writableState.ended = true;
break;

default:
// Probably an error on in uv_guess_handle()
throw new Error('Implement me. Unknown stdin file type!');
}

// For supporting legacy API we put the FD here.
stdin.fd = fd;

// stdin starts out life in a paused state, but node doesn't
// know yet. Explicitly to readStop() it to put it in the
// not-reading state.
if (stdin._handle && stdin._handle.readStop) {
stdin._handle.reading = false;
stdin._readableState.reading = false;
stdin._handle.readStop();
}

// if the user calls stdin.pause(), then we need to stop reading
// immediately, so that the process can close down.
stdin.on('pause', () => {
if (!stdin._handle)
return;
stdin._readableState.reading = false;
stdin._handle.reading = false;
stdin._handle.readStop();
});

// stdin starts out life in a paused state, but node doesn't
// know yet. Explicitly to readStop() it to put it in the
// not-reading state.
if (stdin._handle && stdin._handle.readStop) {
stdin._handle.reading = false;
stdin._readableState.reading = false;
stdin._handle.readStop();
return stdin;
}

// if the user calls stdin.pause(), then we need to stop reading
// immediately, so that the process can close down.
stdin.on('pause', function() {
if (!stdin._handle)
return;
stdin._readableState.reading = false;
stdin._handle.reading = false;
stdin._handle.readStop();
});

return stdin;
});

process.openStdin = function() {
Expand All @@ -119,26 +138,26 @@ function setupStdio() {

function createWritableStdioStream(fd) {
var stream;
var tty_wrap = process.binding('tty_wrap');
const tty_wrap = process.binding('tty_wrap');

// Note stream._type is used for test-module-load-list.js

switch (tty_wrap.guessHandleType(fd)) {
case 'TTY':
var tty = require('tty');
const tty = require('tty');
stream = new tty.WriteStream(fd);
stream._type = 'tty';
break;

case 'FILE':
var fs = require('fs');
const fs = require('fs');
stream = new fs.SyncWriteStream(fd, { autoClose: false });
stream._type = 'fs';
break;

case 'PIPE':
case 'TCP':
var net = require('net');
const net = require('net');
stream = new net.Socket({
fd: fd,
readable: false,
Expand All @@ -159,3 +178,19 @@ function createWritableStdioStream(fd) {

return stream;
}

function createNullStream(fd) {
// Used to approximate /dev/null
const Writable = require('stream').Writable;
const util = require('util');
function NullStream(fd) {
Writable.call(this, {});
this.fd = fd;
this._type = 'null';
}
util.inherits(NullStream, Writable);
NullStream.prototype._write = function(cb) {
setImmediate(cb);
};
return new NullStream(fd);
}
0 ...sues/test-process-external-stdio-close.js → ...llel/test-process-external-stdio-close.js
View file
Open in desktop
File renamed without changes.
11 changes: 1 addition & 10 deletions test/parallel/test-tty-stdout-end.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,4 @@
require('../common');
const assert = require('assert');

const shouldThrow = function() {
process.stdout.end();
};

const validateError = function(e) {
return e instanceof Error &&
e.message === 'process.stdout cannot be closed.';
};

assert.throws(shouldThrow, validateError);
assert.throws(() => process.stdout.end(), /process.stdout cannot be closed./);