Skip to content

Commit

Permalink
errors: make sure all Node.js errors show their properties
Browse files Browse the repository at this point in the history 
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: #29677
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
  • Loading branch information
@BridgeAR
BridgeAR committed Oct 9, 2019
1 parent bf1727a commit 87fb1c2
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 85 deletions.
139 changes: 67 additions & 72 deletions lib/internal/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@

const { Object, Math } = primordials;

const kCode = Symbol('code');
const kInfo = Symbol('info');
const messages = new Map();
const codes = {};

Expand Down Expand Up @@ -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];
}

get errno() {
return this[kInfo].errno;
}

set errno(val) {
this[kInfo].errno = val;
}

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;
}
Object.defineProperty(this, 'errno', {
get() {
return context.errno;
},
set: (value) => {
context.errno = value;
},
enumerable: true,
configurable: true
});

set path(val) {
this[kInfo].path = val ?
lazyBuffer().from(val.toString()) : undefined;
}
Object.defineProperty(this, 'syscall', {
get() {
return context.syscall;
},
set: (value) => {
context.syscall = value;
},
enumerable: true,
configurable: true
});

get dest() {
return this[kInfo].path !== undefined ?
this[kInfo].dest.toString() : undefined;
}
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
});
}

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) {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion test/message/internal_assert.out
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}
4 changes: 3 additions & 1 deletion test/message/internal_assert_fail.out
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}
23 changes: 20 additions & 3 deletions test/parallel/test-dgram-socket-buffer-size.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand All @@ -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();
Expand Down
12 changes: 6 additions & 6 deletions test/parallel/test-internal-errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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);
}

Expand All @@ -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, '');
Expand All @@ -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);
Expand Down
6 changes: 4 additions & 2 deletions test/parallel/test-repl-top-level-await.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
]);
}
Expand Down

0 comments on commit 87fb1c2

Please sign in to comment.