From 26bf70edf47ce90be5c6b58fb68a53e3654708e6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 16 Dec 2017 01:25:17 -0200 Subject: [PATCH 1/6] console: make error handling engine agnostic Calling write could throw a maximum call stack size error. To make sure this is not specific to a single engine (version), lazily populate the correct error message by producing such a error on demand. --- lib/console.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/console.js b/lib/console.js index 8be06935de4839..8480ccfd468c47 100644 --- a/lib/console.js +++ b/lib/console.js @@ -28,6 +28,8 @@ const kCounts = Symbol('counts'); // Track amount of indentation required via `console.group()`. const kGroupIndent = Symbol('groupIndent'); +let MAX_STACK_MESSAGE; + function Console(stdout, stderr, ignoreErrors = true) { if (!(this instanceof Console)) { return new Console(stdout, stderr, ignoreErrors); @@ -111,9 +113,17 @@ function write(ignoreErrors, stream, string, errorhandler, groupIndent) { stream.write(string, errorhandler); } catch (e) { + if (MAX_STACK_MESSAGE === undefined) { + try { + // eslint-disable-next-line no-unused-vars + function a() { a(); } + } catch (err) { + MAX_STACK_MESSAGE = err.message; + } + } // console is a debugging utility, so it swallowing errors is not desirable // even in edge cases such as low stack space. - if (e.message === 'Maximum call stack size exceeded') + if (e.message === MAX_STACK_MESSAGE && e.name === 'RangeError') throw e; // Sorry, there’s no proper way to pass along the error here. } finally { From 6145af19848b697c8593b4ee3fb75b5923790b41 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 16 Dec 2017 01:29:19 -0200 Subject: [PATCH 2/6] console: make variables and checks stricter --- lib/console.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/console.js b/lib/console.js index 8480ccfd468c47..654b5b8280e930 100644 --- a/lib/console.js +++ b/lib/console.js @@ -52,7 +52,7 @@ function Console(stdout, stderr, ignoreErrors = true) { Object.defineProperty(this, '_stdout', prop); prop.value = stderr; Object.defineProperty(this, '_stderr', prop); - prop.value = ignoreErrors; + prop.value = Boolean(ignoreErrors); Object.defineProperty(this, '_ignoreErrors', prop); prop.value = new Map(); Object.defineProperty(this, '_times', prop); @@ -80,7 +80,7 @@ function createWriteErrorHandler(stream) { // 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 (err !== null && !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 @@ -102,7 +102,7 @@ function write(ignoreErrors, stream, string, errorhandler, groupIndent) { } string += '\n'; - if (!ignoreErrors) return stream.write(string); + if (ignoreErrors === false) 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 From c511a5d2e96fc2c4b54d7f420f96b20084300d56 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 16 Dec 2017 01:33:56 -0200 Subject: [PATCH 3/6] benchmark: refactor console benchmark Fix and refactor the console benchmark. It did not test console so it got renamed and mainly tests the different ways of passing arguments through. --- benchmark/misc/arguments.js | 72 +++++++++++++++++++++ benchmark/misc/console.js | 126 ------------------------------------ lib/console.js | 6 +- 3 files changed, 75 insertions(+), 129 deletions(-) create mode 100644 benchmark/misc/arguments.js delete mode 100644 benchmark/misc/console.js diff --git a/benchmark/misc/arguments.js b/benchmark/misc/arguments.js new file mode 100644 index 00000000000000..20de625501bd53 --- /dev/null +++ b/benchmark/misc/arguments.js @@ -0,0 +1,72 @@ +'use strict'; + +const { createBenchmark } = require('../common.js'); +const { Writable } = require('stream'); +const { format, inherits } = require('util'); + +const methods = [ + 'restAndSpread', + 'argumentsAndApply', + 'restAndApply', + 'predefined' +]; + +const bench = createBenchmark(main, { + method: methods, + n: [1e6] +}); + +const nullStream = createNullStream(); + +function usingRestAndSpread(...args) { + nullStream.write(format(...args)); +} + +function usingRestAndApply(...args) { + nullStream.write(format.apply(null, args)); +} + +function usingArgumentsAndApply() { + nullStream.write(format.apply(null, arguments)); +} + +function usingPredefined() { + nullStream.write(format('part 1', 'part', 2, 'part 3', 'part', 4)); +} + +function main({ n, method, args }) { + var fn; + switch (method) { + // '' is a default case for tests + case '': + case 'restAndSpread': + fn = usingRestAndSpread; + break; + case 'restAndApply': + fn = usingRestAndApply; + break; + case 'argumentsAndApply': + fn = usingArgumentsAndApply; + break; + case 'predefined': + fn = usingPredefined; + break; + default: + throw new Error(`Unexpected method "${method}"`); + } + + bench.start(); + for (var i = 0; i < n; i++) + fn('part 1', 'part', 2, 'part 3', 'part', 4); + bench.end(n); +} + +function createNullStream() { + // Used to approximate /dev/null + function NullStream() { + Writable.call(this, {}); + } + inherits(NullStream, Writable); + NullStream.prototype._write = function() {}; + return new NullStream(); +} diff --git a/benchmark/misc/console.js b/benchmark/misc/console.js deleted file mode 100644 index ab938168ac30c4..00000000000000 --- a/benchmark/misc/console.js +++ /dev/null @@ -1,126 +0,0 @@ -'use strict'; - -const common = require('../common.js'); -const assert = require('assert'); -const Writable = require('stream').Writable; -const util = require('util'); - -const methods = [ - 'restAndSpread', - 'argumentsAndApply', - 'restAndApply', - 'restAndConcat' -]; - -const bench = common.createBenchmark(main, { - method: methods, - concat: [1, 0], - n: [1000000] -}); - -const nullStream = createNullStream(); - -function usingRestAndConcat(...args) { - nullStream.write(`this is ${args[0]} of ${args[1]}\n`); -} - -function usingRestAndSpreadTS(...args) { - nullStream.write(`${util.format(...args)}\n`); -} - -function usingRestAndApplyTS(...args) { - nullStream.write(`${util.format.apply(null, args)}\n`); -} - -function usingArgumentsAndApplyTS() { - nullStream.write(`${util.format.apply(null, arguments)}\n`); -} - -function usingRestAndSpreadC(...args) { - nullStream.write(`${util.format(...args)}\n`); -} - -function usingRestAndApplyC(...args) { - nullStream.write(`${util.format.apply(null, args)}\n`); -} - -function usingArgumentsAndApplyC() { - nullStream.write(`${util.format.apply(null, arguments)}\n`); -} - -function runUsingRestAndConcat(n) { - - var i = 0; - bench.start(); - for (; i < n; i++) - usingRestAndConcat('a', 1); - bench.end(n); -} - -function runUsingRestAndSpread(n, concat) { - - const method = concat ? usingRestAndSpreadC : usingRestAndSpreadTS; - - var i = 0; - bench.start(); - for (; i < n; i++) - method('this is %s of %d', 'a', 1); - bench.end(n); -} - -function runUsingRestAndApply(n, concat) { - - const method = concat ? usingRestAndApplyC : usingRestAndApplyTS; - - var i = 0; - bench.start(); - for (; i < n; i++) - method('this is %s of %d', 'a', 1); - bench.end(n); -} - -function runUsingArgumentsAndApply(n, concat) { - - const method = concat ? usingArgumentsAndApplyC : usingArgumentsAndApplyTS; - - var i = 0; - bench.start(); - for (; i < n; i++) - method('this is %s of %d', 'a', 1); - bench.end(n); -} - -function main(conf) { - const n = +conf.n; - switch (conf.method) { - // '' is a default case for tests - case '': - case 'restAndSpread': - runUsingRestAndSpread(n, conf.concat); - break; - case 'restAndApply': - runUsingRestAndApply(n, conf.concat); - break; - case 'argumentsAndApply': - runUsingArgumentsAndApply(n, conf.concat); - break; - case 'restAndConcat': - if (conf.concat) - runUsingRestAndConcat(n); - break; - default: - throw new Error('Unexpected method'); - } -} - -function createNullStream() { - // Used to approximate /dev/null - function NullStream() { - Writable.call(this, {}); - } - util.inherits(NullStream, Writable); - NullStream.prototype._write = function(cb) { - assert.strictEqual(cb.toString(), 'this is a of 1\n'); - }; - return new NullStream(); -} diff --git a/lib/console.js b/lib/console.js index 654b5b8280e930..ae268d8f8417b0 100644 --- a/lib/console.js +++ b/lib/console.js @@ -132,12 +132,12 @@ function write(ignoreErrors, stream, string, errorhandler, groupIndent) { } -// 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) { write(this._ignoreErrors, this._stdout, + // The performance of .apply and the spread operator seems on par in V8 + // 6.3 but the spread operator, unlike .apply(), pushes the elements + // onto the stack. That is, it makes stack overflows more likely. util.format.apply(null, args), this._stdoutErrorHandler, this[kGroupIndent]); From a4e3aa5e2a55238ab9d95de33247b29c6e149bfb Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 16 Dec 2017 01:36:23 -0200 Subject: [PATCH 4/6] console: order functions and remove \n\n This lists all function aliases directly below the declared function. --- lib/console.js | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/lib/console.js b/lib/console.js index ae268d8f8417b0..cbaef074046f9c 100644 --- a/lib/console.js +++ b/lib/console.js @@ -131,7 +131,6 @@ function write(ignoreErrors, stream, string, errorhandler, groupIndent) { } } - Console.prototype.log = function log(...args) { write(this._ignoreErrors, this._stdout, @@ -142,13 +141,9 @@ Console.prototype.log = function log(...args) { this._stdoutErrorHandler, this[kGroupIndent]); }; - - Console.prototype.debug = Console.prototype.log; - - Console.prototype.info = Console.prototype.log; - +Console.prototype.dirxml = Console.prototype.log; Console.prototype.warn = function warn(...args) { write(this._ignoreErrors, @@ -157,11 +152,8 @@ Console.prototype.warn = function warn(...args) { this._stderrErrorHandler, this[kGroupIndent]); }; - - Console.prototype.error = Console.prototype.warn; - Console.prototype.dir = function dir(object, options) { options = Object.assign({ customInspect: false }, options); write(this._ignoreErrors, @@ -171,17 +163,12 @@ Console.prototype.dir = function dir(object, options) { this[kGroupIndent]); }; - -Console.prototype.dirxml = Console.prototype.log; - - Console.prototype.time = function time(label = 'default') { // Coerces everything other than Symbol to a string label = `${label}`; this._times.set(label, process.hrtime()); }; - Console.prototype.timeEnd = function timeEnd(label = 'default') { // Coerces everything other than Symbol to a string label = `${label}`; @@ -196,7 +183,6 @@ Console.prototype.timeEnd = function timeEnd(label = 'default') { this._times.delete(label); }; - Console.prototype.trace = function trace(...args) { const err = { name: 'Trace', @@ -206,7 +192,6 @@ Console.prototype.trace = function trace(...args) { this.error(err.stack); }; - Console.prototype.assert = function assert(expression, ...args) { if (!expression) { require('assert').ok(false, util.format.apply(null, args)); @@ -256,7 +241,6 @@ Console.prototype.group = function group(...data) { } this[kGroupIndent] += ' '; }; - Console.prototype.groupCollapsed = Console.prototype.group; Console.prototype.groupEnd = function groupEnd() { From 2dc5813209c6c97f92dafe1282426b3619cd90eb Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 16 Dec 2017 03:21:15 -0200 Subject: [PATCH 5/6] doc: remove old console note This should not be important anymore in newer docs. The change is also documented in the "changed" part of the function. --- doc/api/console.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/doc/api/console.md b/doc/api/console.md index 5722f362841484..84b7e2c11dac8e 100644 --- a/doc/api/console.md +++ b/doc/api/console.md @@ -402,11 +402,6 @@ console.timeEnd('100-elements'); // prints 100-elements: 225.438ms ``` -*Note*: As of Node.js v6.0.0, `console.timeEnd()` deletes the timer to avoid -leaking it. On older versions, the timer persisted. This allowed -`console.timeEnd()` to be called multiple times for the same label. This -functionality was unintended and is no longer supported. - ### console.trace([message][, ...args])