Skip to content
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

console: do not emit error events #9744

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
58 changes: 53 additions & 5 deletions lib/console.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

const util = require('util');

function Console(stdout, stderr) {
function Console(stdout, stderr, ignoreErrors = true) {
if (!(this instanceof Console)) {
return new Console(stdout, stderr);
return new Console(stdout, stderr, ignoreErrors);
}
if (!stdout || typeof stdout.write !== 'function') {
throw new TypeError('Console expects a writable stream instance');
Expand All @@ -24,8 +24,14 @@ function Console(stdout, stderr) {
Object.defineProperty(this, '_stdout', prop);
prop.value = stderr;
Object.defineProperty(this, '_stderr', prop);
prop.value = ignoreErrors;
Object.defineProperty(this, '_ignoreErrors', prop);
prop.value = new Map();
Object.defineProperty(this, '_times', prop);
prop.value = createWriteErrorHandler(stdout);
Object.defineProperty(this, '_stdoutErrorHandler', prop);
prop.value = createWriteErrorHandler(stderr);
Object.defineProperty(this, '_stderrErrorHandler', prop);

// bind the prototype functions to this Console instance
var keys = Object.keys(Console.prototype);
Expand All @@ -35,20 +41,60 @@ function Console(stdout, stderr) {
}
}

// Make a function that can serve as the callback passed to `stream.write()`.
function createWriteErrorHandler(stream) {
return (err) => {
// This conditional evaluates to true if and only if there was an error
// that was not already emitted (which happens when the _write callback
// is invoked asynchronously).
if (err && !stream._writableState.errorEmitted) {
// If there was an error, it will be emitted on `stream` as
// an `error` event. Adding a `once` listener will keep that error
// from becoming an uncaught exception, but since the handler is
// removed after the event, non-console.* writes won’t be affected.
stream.once('error', noop);
}
};
}

function write(ignoreErrors, stream, string, errorhandler) {
if (!ignoreErrors) return stream.write(string);

// There may be an error occurring synchronously (e.g. for files or TTYs
// on POSIX systems) or asynchronously (e.g. pipes on POSIX systems), so
// handle both situations.
try {
// Add and later remove a noop error handler to catch synchronous errors.
stream.once('error', noop);

stream.write(string, errorhandler);
} catch (e) {
// Sorry, there’s no proper way to pass along the error here.
} finally {
stream.removeListener('error', noop);
}
}


// As of v8 5.0.71.32, the combination of rest param, template string
// and .apply(null, args) benchmarks consistently faster than using
// the spread operator when calling util.format.
Console.prototype.log = function log(...args) {
this._stdout.write(`${util.format.apply(null, args)}\n`);
write(this._ignoreErrors,
this._stdout,
`${util.format.apply(null, args)}\n`,
this._stdoutErrorHandler);
};


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


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


Expand All @@ -57,7 +103,7 @@ Console.prototype.error = Console.prototype.warn;

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


Expand Down Expand Up @@ -99,3 +145,5 @@ Console.prototype.assert = function assert(expression, ...args) {

module.exports = new Console(process.stdout, process.stderr);
module.exports.Console = Console;

function noop() {}
17 changes: 17 additions & 0 deletions test/parallel/test-console-async-write-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';
const common = require('../common');
const { Console } = require('console');
const { Writable } = require('stream');
const assert = require('assert');

const out = new Writable({
write: common.mustCall((chunk, enc, callback) => {
process.nextTick(callback, new Error('foobar'));
})
});

const c = new Console(out, out, true);

assert.doesNotThrow(() => {
c.log('abc');
});
47 changes: 47 additions & 0 deletions test/parallel/test-console-sync-write-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

It would be helpful to have additional tests that verify this works with a variety of stdout/stderr configurations (e.g. stdio as a file, stdio as a pipe, etc)

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 updated this with tests for something that errors asynchronously… the inner workings of console aren’t really tied to I/O at all, so I’m not sure it makes sense to add explicit tests for these

Copy link
Member

Choose a reason for hiding this comment

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

The main thing that I'm hoping for is that this would fix the issue reported in #831

const common = require('../common');
const { Console } = require('console');
const { Writable } = require('stream');
const assert = require('assert');

{
const out = new Writable({
write: common.mustCall((chunk, enc, callback) => {
callback(new Error('foobar'));
})
});

const c = new Console(out, out, true);

assert.doesNotThrow(() => {
c.log('abc');
});
}

{
const out = new Writable({
write: common.mustCall((chunk, enc, callback) => {
throw new Error('foobar');
})
});

const c = new Console(out, out, true);

assert.doesNotThrow(() => {
c.log('abc');
});
}

{
const out = new Writable({
write: common.mustCall((chunk, enc, callback) => {
setImmediate(() => callback(new Error('foobar')));
})
});

const c = new Console(out, out, true);

assert.doesNotThrow(() => {
c.log('abc');
});
}