From 500720f5781b96147422614b82c1f85e22d6a6e8 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 23 Sep 2019 17:22:10 +0200 Subject: [PATCH] errors: make sure all Node.js errors show their properties This improves Node.js errors by always showing the attached properties when inspecting such an error. This applies especially to SystemError. It did often not show any properties but now all properties will be visible. This is done in a mainly backwards compatible way. Instead of using prototype getters and setters, the property is now set directly on the error. PR-URL: https://github.com/nodejs/node/pull/29677 Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum --- lib/internal/errors.js | 139 +++++++++--------- test/message/internal_assert.out | 4 +- test/message/internal_assert_fail.out | 4 +- .../parallel/test-dgram-socket-buffer-size.js | 23 ++- test/parallel/test-internal-errors.js | 12 +- test/parallel/test-repl-top-level-await.js | 6 +- 6 files changed, 103 insertions(+), 85 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 8bfe3acb7b4..eb3c435b20e 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -12,8 +12,6 @@ const { Object, Math } = primordials; -const kCode = Symbol('code'); -const kInfo = Symbol('info'); const messages = new Map(); const codes = {}; @@ -121,76 +119,86 @@ class SystemError extends Error { writable: true, configurable: true }); - Object.defineProperty(this, kInfo, { - configurable: false, - enumerable: false, - value: context - }); - Object.defineProperty(this, kCode, { - configurable: true, - enumerable: false, - value: key, - writable: true - }); addCodeToName(this, 'SystemError', key); - } - get code() { - return this[kCode]; - } + this.code = key; - set code(value) { - Object.defineProperty(this, 'code', { - configurable: true, + Object.defineProperty(this, 'info', { + value: context, enumerable: true, - value, - writable: true + configurable: true, + writable: false }); - } - get info() { - return this[kInfo]; - } + Object.defineProperty(this, 'errno', { + get() { + return context.errno; + }, + set: (value) => { + context.errno = value; + }, + enumerable: true, + configurable: true + }); - get errno() { - return this[kInfo].errno; - } + Object.defineProperty(this, 'syscall', { + get() { + return context.syscall; + }, + set: (value) => { + context.syscall = value; + }, + enumerable: true, + configurable: true + }); - set errno(val) { - this[kInfo].errno = val; - } + if (context.path !== undefined) { + // TODO(BridgeAR): Investigate why and when the `.toString()` was + // introduced. The `path` and `dest` properties in the context seem to + // always be of type string. We should probably just remove the + // `.toString()` and `Buffer.from()` operations and set the value on the + // context as the user did. + Object.defineProperty(this, 'path', { + get() { + return context.path != null ? + context.path.toString() : context.path; + }, + set: (value) => { + context.path = value ? + lazyBuffer().from(value.toString()) : undefined; + }, + enumerable: true, + configurable: true + }); + } - get syscall() { - return this[kInfo].syscall; - } - - set syscall(val) { - this[kInfo].syscall = val; - } - - get path() { - return this[kInfo].path !== undefined ? - this[kInfo].path.toString() : undefined; - } - - set path(val) { - this[kInfo].path = val ? - lazyBuffer().from(val.toString()) : undefined; - } - - get dest() { - return this[kInfo].path !== undefined ? - this[kInfo].dest.toString() : undefined; - } - - set dest(val) { - this[kInfo].dest = val ? - lazyBuffer().from(val.toString()) : undefined; + if (context.dest !== undefined) { + Object.defineProperty(this, 'dest', { + get() { + return context.dest != null ? + context.dest.toString() : context.dest; + }, + set: (value) => { + context.dest = value ? + lazyBuffer().from(value.toString()) : undefined; + }, + enumerable: true, + configurable: true + }); + } } toString() { return `${this.name} [${this.code}]: ${this.message}`; } + + [Symbol.for('nodejs.util.inspect.custom')](recurseTimes, ctx) { + return lazyInternalUtilInspect().inspect(this, { + ...ctx, + getters: true, + customInspect: false + }); + } } function makeSystemErrorWithCode(key) { @@ -221,19 +229,7 @@ function makeNodeErrorWithCode(Base, key) { configurable: true }); addCodeToName(this, super.name, key); - } - - get code() { - return key; - } - - set code(value) { - Object.defineProperty(this, 'code', { - configurable: true, - enumerable: true, - value, - writable: true - }); + this.code = key; } toString() { @@ -394,7 +390,6 @@ function uvException(ctx) { err[prop] = ctx[prop]; } - // TODO(BridgeAR): Show the `code` property as part of the stack. err.code = code; if (path) { err.path = path; diff --git a/test/message/internal_assert.out b/test/message/internal_assert.out index ae8de3e1a0d..cf09fdcb605 100644 --- a/test/message/internal_assert.out +++ b/test/message/internal_assert.out @@ -12,4 +12,6 @@ Please open an issue with this stack trace at https://github.com/nodejs/node/iss at * at * at * - at * + at * { + code: 'ERR_INTERNAL_ASSERTION' +} diff --git a/test/message/internal_assert_fail.out b/test/message/internal_assert_fail.out index 70f49ad33aa..11b532b7b2a 100644 --- a/test/message/internal_assert_fail.out +++ b/test/message/internal_assert_fail.out @@ -13,4 +13,6 @@ Please open an issue with this stack trace at https://github.com/nodejs/node/iss at * at * at * - at * + at * { + code: 'ERR_INTERNAL_ASSERTION' +} diff --git a/test/parallel/test-dgram-socket-buffer-size.js b/test/parallel/test-dgram-socket-buffer-size.js index c2936ba8fd9..d3317e85c3d 100644 --- a/test/parallel/test-dgram-socket-buffer-size.js +++ b/test/parallel/test-dgram-socket-buffer-size.js @@ -4,6 +4,7 @@ const common = require('../common'); const assert = require('assert'); const dgram = require('dgram'); +const { inspect } = require('util'); const { SystemError } = require('internal/errors'); const { internalBinding } = require('internal/test/binding'); const { @@ -22,7 +23,7 @@ function getExpectedError(type) { 'ENOTSOCK (socket operation on non-socket)' : 'EBADF (bad file descriptor)'; const error = { code: 'ERR_SOCKET_BUFFER_SIZE', - type: SystemError, + name: 'SystemError', message: `Could not get or set buffer size: ${syscall} returned ${suffix}`, info: { code, @@ -40,9 +41,25 @@ function getExpectedError(type) { const socket = dgram.createSocket('udp4'); - common.expectsError(() => { + assert.throws(() => { socket.setSendBufferSize(8192); - }, errorObj); + }, (err) => { + assert.strictEqual( + inspect(err).replace(/^ +at .*\n/gm, ''), + `SystemError [ERR_SOCKET_BUFFER_SIZE]: ${errorObj.message}\n` + + " code: 'ERR_SOCKET_BUFFER_SIZE',\n" + + ' info: {\n' + + ` errno: ${errorObj.info.errno},\n` + + ` code: '${errorObj.info.code}',\n` + + ` message: '${errorObj.info.message}',\n` + + ` syscall: '${errorObj.info.syscall}'\n` + + ' },\n' + + ` errno: [Getter/Setter: ${errorObj.info.errno}],\n` + + ` syscall: [Getter/Setter: '${errorObj.info.syscall}']\n` + + '}' + ); + return true; + }); common.expectsError(() => { socket.getSendBufferSize(); diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index 10d79cb8faf..a34218fd1dd 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -82,9 +82,9 @@ common.expectsError(() => { { const myError = new errors.codes.TEST_ERROR_1('foo'); assert.strictEqual(myError.code, 'TEST_ERROR_1'); - assert.strictEqual(myError.hasOwnProperty('code'), false); + assert.strictEqual(myError.hasOwnProperty('code'), true); assert.strictEqual(myError.hasOwnProperty('name'), false); - assert.deepStrictEqual(Object.keys(myError), []); + assert.deepStrictEqual(Object.keys(myError), ['code']); const initialName = myError.name; myError.code = 'FHQWHGADS'; assert.strictEqual(myError.code, 'FHQWHGADS'); @@ -99,11 +99,11 @@ common.expectsError(() => { // browser. Note that `name` becomes enumerable after being assigned. { const myError = new errors.codes.TEST_ERROR_1('foo'); - assert.deepStrictEqual(Object.keys(myError), []); + assert.deepStrictEqual(Object.keys(myError), ['code']); const initialToString = myError.toString(); myError.name = 'Fhqwhgads'; - assert.deepStrictEqual(Object.keys(myError), ['name']); + assert.deepStrictEqual(Object.keys(myError), ['code', 'name']); assert.notStrictEqual(myError.toString(), initialToString); } @@ -114,7 +114,7 @@ common.expectsError(() => { let initialConsoleLog = ''; hijackStdout((data) => { initialConsoleLog += data; }); const myError = new errors.codes.TEST_ERROR_1('foo'); - assert.deepStrictEqual(Object.keys(myError), []); + assert.deepStrictEqual(Object.keys(myError), ['code']); const initialToString = myError.toString(); console.log(myError); assert.notStrictEqual(initialConsoleLog, ''); @@ -124,7 +124,7 @@ common.expectsError(() => { let subsequentConsoleLog = ''; hijackStdout((data) => { subsequentConsoleLog += data; }); myError.message = 'Fhqwhgads'; - assert.deepStrictEqual(Object.keys(myError), []); + assert.deepStrictEqual(Object.keys(myError), ['code']); assert.notStrictEqual(myError.toString(), initialToString); console.log(myError); assert.strictEqual(subsequentConsoleLog, initialConsoleLog); diff --git a/test/parallel/test-repl-top-level-await.js b/test/parallel/test-repl-top-level-await.js index f72a5daa015..f629d3f1d0d 100644 --- a/test/parallel/test-repl-top-level-await.js +++ b/test/parallel/test-repl-top-level-await.js @@ -161,8 +161,10 @@ async function ctrlCTest() { ]), [ 'await timeout(100000)\r', 'Thrown:', - 'Error [ERR_SCRIPT_EXECUTION_INTERRUPTED]: ' + - 'Script execution was interrupted by `SIGINT`', + '[Error [ERR_SCRIPT_EXECUTION_INTERRUPTED]: ' + + 'Script execution was interrupted by `SIGINT`] {', + " code: 'ERR_SCRIPT_EXECUTION_INTERRUPTED'", + '}', PROMPT ]); }