From f18e08d820dde161788d95a5603546ceca021e90 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 22 Nov 2016 18:21:43 +0100 Subject: [PATCH] console: do not emit error events Fixes: https://github.com/nodejs/node/issues/831 Fixes: https://github.com/nodejs/node/issues/947 Ref: https://github.com/nodejs/node/pull/9470 PR-URL: https://github.com/nodejs/node/pull/9744 Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Sakthipriyan Vairamani --- lib/console.js | 58 +++++++++++++++++-- .../test-console-async-write-error.js | 17 ++++++ .../parallel/test-console-sync-write-error.js | 47 +++++++++++++++ .../test-process-external-stdio-close.js | 0 4 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-console-async-write-error.js create mode 100644 test/parallel/test-console-sync-write-error.js rename test/{known_issues => parallel}/test-process-external-stdio-close.js (100%) diff --git a/lib/console.js b/lib/console.js index ba92c91636ba38..7c4e4bb786e180 100644 --- a/lib/console.js +++ b/lib/console.js @@ -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'); @@ -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); @@ -35,12 +41,49 @@ 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); }; @@ -48,7 +91,10 @@ 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); }; @@ -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`); }; @@ -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() {} diff --git a/test/parallel/test-console-async-write-error.js b/test/parallel/test-console-async-write-error.js new file mode 100644 index 00000000000000..0fcd72868ab090 --- /dev/null +++ b/test/parallel/test-console-async-write-error.js @@ -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'); +}); diff --git a/test/parallel/test-console-sync-write-error.js b/test/parallel/test-console-sync-write-error.js new file mode 100644 index 00000000000000..34ff8bad8c9f3d --- /dev/null +++ b/test/parallel/test-console-sync-write-error.js @@ -0,0 +1,47 @@ +'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) => { + 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'); + }); +} diff --git a/test/known_issues/test-process-external-stdio-close.js b/test/parallel/test-process-external-stdio-close.js similarity index 100% rename from test/known_issues/test-process-external-stdio-close.js rename to test/parallel/test-process-external-stdio-close.js