From 640f32bce9fef8b6fdf979b41cf17fa444f9a36e Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Mon, 8 May 2023 21:28:33 +0300 Subject: [PATCH 1/6] worker: support more cases when (de)serializing errors - error.cause is potentially an error, so is now handled recursively - best effort to serialize thrown symbols - handle thrown object with custom inspect --- lib/internal/error_serdes.js | 55 ++++++++++++++++++++++++++---- test/parallel/test-error-serdes.js | 23 +++++++++++++ 2 files changed, 71 insertions(+), 7 deletions(-) diff --git a/lib/internal/error_serdes.js b/lib/internal/error_serdes.js index 13f3f8b35fdab0..7eb1077173eb9e 100644 --- a/lib/internal/error_serdes.js +++ b/lib/internal/error_serdes.js @@ -13,19 +13,27 @@ const { ObjectGetOwnPropertyNames, ObjectGetPrototypeOf, ObjectKeys, + ObjectPrototypeHasOwnProperty, ObjectPrototypeToString, RangeError, ReferenceError, SafeSet, + StringPrototypeSubstring, SymbolToStringTag, SyntaxError, + SymbolFor, TypeError, URIError, } = primordials; +const { inspect: { custom: customInspectSymbol } } = require('util'); const kSerializedError = 0; const kSerializedObject = 1; const kInspectedError = 2; +const kInspectedSymbol = 3; +const kCustomInspectedObject = 4; + +const kSymbolStringLength = 'Symbol('.length; const errors = { Error, TypeError, RangeError, URIError, SyntaxError, ReferenceError, EvalError, @@ -52,7 +60,13 @@ function TryGetAllProperties(object, target = object) { // Continue regardless of error. } } - if ('value' in descriptor && typeof descriptor.value !== 'function') { + if (key === 'cause') { + delete descriptor.get; + delete descriptor.set; + descriptor.value = serializeError(descriptor.value); + all[key] = descriptor; + } else if ('value' in descriptor && + typeof descriptor.value !== 'function' && typeof descriptor.value !== 'symbol') { delete descriptor.get; delete descriptor.set; all[key] = descriptor; @@ -95,6 +109,10 @@ function inspect(...args) { let serialize; function serializeError(error) { if (!serialize) serialize = require('v8').serialize; + if (typeof error === 'symbol') { + return Buffer.concat([Buffer.from([kInspectedSymbol]), + Buffer.from(inspect(error), 'utf8')]); + } try { if (typeof error === 'object' && ObjectPrototypeToString(error) === '[object Error]') { @@ -113,6 +131,15 @@ function serializeError(error) { } catch { // Continue regardless of error. } + try { + if (error != null && + ObjectPrototypeHasOwnProperty(error, customInspectSymbol)) { + return Buffer.concat([Buffer.from([kCustomInspectedObject]), + Buffer.from(inspect(error), 'utf8')]); + } + } catch { + // Continue regardless of error. + } try { const serialized = serialize(error); return Buffer.concat([Buffer.from([kSerializedObject]), serialized]); @@ -123,6 +150,12 @@ function serializeError(error) { Buffer.from(inspect(error), 'utf8')]); } +function fromBuffer(error) { + return Buffer.from(error.buffer, + error.byteOffset + 1, + error.byteLength - 1); +} + let deserialize; function deserializeError(error) { if (!deserialize) deserialize = require('v8').deserialize; @@ -132,19 +165,27 @@ function deserializeError(error) { const ctor = errors[constructor]; ObjectDefineProperty(properties, SymbolToStringTag, { __proto__: null, - value: { value: 'Error', configurable: true }, + value: { __proto__: null, value: 'Error', configurable: true }, enumerable: true, }); + if ('cause' in properties && 'value' in properties.cause) { + properties.cause.value = deserializeError(properties.cause.value); + } return ObjectCreate(ctor.prototype, properties); } case kSerializedObject: return deserialize(error.subarray(1)); - case kInspectedError: { - const buf = Buffer.from(error.buffer, - error.byteOffset + 1, - error.byteLength - 1); - return buf.toString('utf8'); + case kInspectedError: + return fromBuffer(error).toString('utf8'); + case kInspectedSymbol: { + const buf = fromBuffer(error); + return SymbolFor(StringPrototypeSubstring(buf.toString('utf8'), kSymbolStringLength, buf.length - 1)); } + case kCustomInspectedObject: + return { + __proto__: null, + [customInspectSymbol]: () => fromBuffer(error).toString('utf8'), + }; } require('assert').fail('This should not happen'); } diff --git a/test/parallel/test-error-serdes.js b/test/parallel/test-error-serdes.js index 92d0864348a831..95c57dc7062f5a 100644 --- a/test/parallel/test-error-serdes.js +++ b/test/parallel/test-error-serdes.js @@ -2,6 +2,7 @@ 'use strict'; require('../common'); const assert = require('assert'); +const { inspect } = require('util'); const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes; const { serializeError, deserializeError } = require('internal/error_serdes'); @@ -15,6 +16,9 @@ assert.strictEqual(cycle(1.4), 1.4); assert.strictEqual(cycle(null), null); assert.strictEqual(cycle(undefined), undefined); assert.strictEqual(cycle('foo'), 'foo'); +assert.strictEqual(cycle(Symbol.for('foo')), Symbol.for('foo')); +assert.strictEqual(cycle(Symbol('foo')).toString(), Symbol('foo').toString()); + let err = new Error('foo'); for (let i = 0; i < 10; i++) { @@ -43,6 +47,16 @@ assert.strictEqual(cycle(new SubError('foo')).name, 'Error'); assert.deepStrictEqual(cycle({ message: 'foo' }), { message: 'foo' }); assert.strictEqual(cycle(Function), '[Function: Function]'); + +assert.strictEqual(cycle(new Error('Error with cause', { cause: 0 })).cause, 0); +assert.strictEqual(cycle(new Error('Error with cause', { cause: -1 })).cause, -1); +assert.strictEqual(cycle(new Error('Error with cause', { cause: 1.4 })).cause, 1.4); +assert.strictEqual(cycle(new Error('Error with cause', { cause: null })).cause, null); +assert.strictEqual(cycle(new Error('Error with cause', { cause: undefined })).cause, undefined); +assert.strictEqual(cycle(new Error('Error with cause', { cause: 'foo' })).cause, 'foo'); +assert.deepStrictEqual(cycle(new Error('Error with cause', { cause: new Error('err') })).cause, new Error('err')); + + { const err = new ERR_INVALID_ARG_TYPE('object', 'Object', 42); assert.match(String(err), /^TypeError \[ERR_INVALID_ARG_TYPE\]:/); @@ -66,3 +80,12 @@ assert.strictEqual(cycle(Function), '[Function: Function]'); serializeError(new DynamicError()); assert.strictEqual(called, true); } + + +const data = { + foo: 'bar', + [inspect.custom]() { + return 'barbaz'; + } +}; +assert.strictEqual(inspect(cycle(data)), 'barbaz'); From b814c8c52e7e38eb351dcbe930e360c2d94e9e17 Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Mon, 8 May 2023 22:52:22 +0300 Subject: [PATCH 2/6] add test for cause getter --- test/parallel/test-error-serdes.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/parallel/test-error-serdes.js b/test/parallel/test-error-serdes.js index 95c57dc7062f5a..2153e8c771fed8 100644 --- a/test/parallel/test-error-serdes.js +++ b/test/parallel/test-error-serdes.js @@ -55,6 +55,12 @@ assert.strictEqual(cycle(new Error('Error with cause', { cause: null })).cause, assert.strictEqual(cycle(new Error('Error with cause', { cause: undefined })).cause, undefined); assert.strictEqual(cycle(new Error('Error with cause', { cause: 'foo' })).cause, 'foo'); assert.deepStrictEqual(cycle(new Error('Error with cause', { cause: new Error('err') })).cause, new Error('err')); +class ErrorWithCause extends Error { + get cause() { + return new Error('err'); + } +} +assert.deepStrictEqual(cycle(new ErrorWithCause('Error with cause')).cause, new Error('err')); { From 86b6b6651e22be9e3e05ef40fe38ecf97034ca7d Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Wed, 10 May 2023 21:41:38 +0300 Subject: [PATCH 3/6] CR --- lib/internal/error_serdes.js | 6 ++---- test/parallel/test-error-serdes.js | 9 ++++++++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/internal/error_serdes.js b/lib/internal/error_serdes.js index 7eb1077173eb9e..787adca15d3d30 100644 --- a/lib/internal/error_serdes.js +++ b/lib/internal/error_serdes.js @@ -56,19 +56,17 @@ function TryGetAllProperties(object, target = object) { if (getter && key !== '__proto__') { try { descriptor.value = FunctionPrototypeCall(getter, target); + delete descriptor.get; + delete descriptor.set; } catch { // Continue regardless of error. } } if (key === 'cause') { - delete descriptor.get; - delete descriptor.set; descriptor.value = serializeError(descriptor.value); all[key] = descriptor; } else if ('value' in descriptor && typeof descriptor.value !== 'function' && typeof descriptor.value !== 'symbol') { - delete descriptor.get; - delete descriptor.set; all[key] = descriptor; } }); diff --git a/test/parallel/test-error-serdes.js b/test/parallel/test-error-serdes.js index 2153e8c771fed8..c12797836b6ae5 100644 --- a/test/parallel/test-error-serdes.js +++ b/test/parallel/test-error-serdes.js @@ -53,6 +53,7 @@ assert.strictEqual(cycle(new Error('Error with cause', { cause: -1 })).cause, -1 assert.strictEqual(cycle(new Error('Error with cause', { cause: 1.4 })).cause, 1.4); assert.strictEqual(cycle(new Error('Error with cause', { cause: null })).cause, null); assert.strictEqual(cycle(new Error('Error with cause', { cause: undefined })).cause, undefined); +assert.strictEqual(Object.hasOwn(cycle(new Error('Error with cause', { cause: undefined })), 'cause'), true); assert.strictEqual(cycle(new Error('Error with cause', { cause: 'foo' })).cause, 'foo'); assert.deepStrictEqual(cycle(new Error('Error with cause', { cause: new Error('err') })).cause, new Error('err')); class ErrorWithCause extends Error { @@ -61,7 +62,13 @@ class ErrorWithCause extends Error { } } assert.deepStrictEqual(cycle(new ErrorWithCause('Error with cause')).cause, new Error('err')); - +class ErrorWithThowingCause extends Error { + get cause() { + throw new Error('err'); + } +} +assert.strictEqual(cycle(new ErrorWithThowingCause('Error with cause')).cause, undefined); +assert.strictEqual(Object.hasOwn(cycle(new ErrorWithThowingCause('Error with cause')), 'cause'), false); { const err = new ERR_INVALID_ARG_TYPE('object', 'Object', 42); From 40b0fc190b116182b0dbad2657778d657283d7be Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Thu, 11 May 2023 09:25:34 +0300 Subject: [PATCH 4/6] CR --- test/parallel/test-error-serdes.js | 38 ++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/test/parallel/test-error-serdes.js b/test/parallel/test-error-serdes.js index c12797836b6ae5..60d1dafb244567 100644 --- a/test/parallel/test-error-serdes.js +++ b/test/parallel/test-error-serdes.js @@ -47,28 +47,46 @@ assert.strictEqual(cycle(new SubError('foo')).name, 'Error'); assert.deepStrictEqual(cycle({ message: 'foo' }), { message: 'foo' }); assert.strictEqual(cycle(Function), '[Function: Function]'); - -assert.strictEqual(cycle(new Error('Error with cause', { cause: 0 })).cause, 0); -assert.strictEqual(cycle(new Error('Error with cause', { cause: -1 })).cause, -1); -assert.strictEqual(cycle(new Error('Error with cause', { cause: 1.4 })).cause, 1.4); -assert.strictEqual(cycle(new Error('Error with cause', { cause: null })).cause, null); -assert.strictEqual(cycle(new Error('Error with cause', { cause: undefined })).cause, undefined); -assert.strictEqual(Object.hasOwn(cycle(new Error('Error with cause', { cause: undefined })), 'cause'), true); -assert.strictEqual(cycle(new Error('Error with cause', { cause: 'foo' })).cause, 'foo'); -assert.deepStrictEqual(cycle(new Error('Error with cause', { cause: new Error('err') })).cause, new Error('err')); class ErrorWithCause extends Error { get cause() { return new Error('err'); } } -assert.deepStrictEqual(cycle(new ErrorWithCause('Error with cause')).cause, new Error('err')); class ErrorWithThowingCause extends Error { get cause() { throw new Error('err'); } } +class ErrorWithCyclicCause extends Error { + get cause() { + return new ErrorWithCyclicCause('err'); + } +} + +assert.strictEqual(cycle(new Error('Error with cause', { cause: 0 })).cause, 0); +assert.strictEqual(cycle(new Error('Error with cause', { cause: -1 })).cause, -1); +assert.strictEqual(cycle(new Error('Error with cause', { cause: 1.4 })).cause, 1.4); +assert.strictEqual(cycle(new Error('Error with cause', { cause: null })).cause, null); +assert.strictEqual(cycle(new Error('Error with cause', { cause: undefined })).cause, undefined); +assert.strictEqual(Object.hasOwn(cycle(new Error('Error with cause', { cause: undefined })), 'cause'), true); +assert.strictEqual(cycle(new Error('Error with cause', { cause: 'foo' })).cause, 'foo'); +assert.deepStrictEqual(cycle(new Error('Error with cause', { cause: new Error('err') })).cause, new Error('err')); +assert.deepStrictEqual( + cycle(Object.defineProperty(new Error('Error with cause'), 'cause', { get() { return { foo: 'bar' }; } })).cause, + { foo: 'bar' } +); +assert.deepStrictEqual(cycle(new ErrorWithCause('Error with cause')).cause, new Error('err')); assert.strictEqual(cycle(new ErrorWithThowingCause('Error with cause')).cause, undefined); assert.strictEqual(Object.hasOwn(cycle(new ErrorWithThowingCause('Error with cause')), 'cause'), false); +// When the cause is cyclic, it is serialized until Maxiumum call stack size is reached +let depth = 0; +let e = cycle(new ErrorWithCyclicCause('Error with cause')); +while (e.cause) { + e = e.cause; + depth++; +} +assert(depth > 1); + { const err = new ERR_INVALID_ARG_TYPE('object', 'Object', 42); From 9b5e1246337827916b35a72059dd3e4f101c353e Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Thu, 11 May 2023 19:14:05 +0300 Subject: [PATCH 5/6] CR --- lib/internal/error_serdes.js | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/lib/internal/error_serdes.js b/lib/internal/error_serdes.js index 787adca15d3d30..e4850c63aaffb3 100644 --- a/lib/internal/error_serdes.js +++ b/lib/internal/error_serdes.js @@ -18,11 +18,15 @@ const { RangeError, ReferenceError, SafeSet, + StringFromCharCode, StringPrototypeSubstring, SymbolToStringTag, SyntaxError, SymbolFor, TypeError, + TypedArrayPrototypeGetBuffer, + TypedArrayPrototypeGetByteOffset, + TypedArrayPrototypeGetByteLength, URIError, } = primordials; const { inspect: { custom: customInspectSymbol } } = require('util'); @@ -50,6 +54,7 @@ function TryGetAllProperties(object, target = object) { ArrayPrototypeForEach(keys, (key) => { let descriptor; try { + // TODO: create a null-prototype descriptor with needed properties only descriptor = ObjectGetOwnPropertyDescriptor(object, key); } catch { return; } const getter = descriptor.get; @@ -108,8 +113,7 @@ let serialize; function serializeError(error) { if (!serialize) serialize = require('v8').serialize; if (typeof error === 'symbol') { - return Buffer.concat([Buffer.from([kInspectedSymbol]), - Buffer.from(inspect(error), 'utf8')]); + return Buffer.from(StringFromCharCode(kInspectedSymbol) + inspect(error), 'utf8'); } try { if (typeof error === 'object' && @@ -132,8 +136,7 @@ function serializeError(error) { try { if (error != null && ObjectPrototypeHasOwnProperty(error, customInspectSymbol)) { - return Buffer.concat([Buffer.from([kCustomInspectedObject]), - Buffer.from(inspect(error), 'utf8')]); + return Buffer.from(StringFromCharCode(kCustomInspectedObject) + inspect(error), 'utf8'); } } catch { // Continue regardless of error. @@ -144,14 +147,13 @@ function serializeError(error) { } catch { // Continue regardless of error. } - return Buffer.concat([Buffer.from([kInspectedError]), - Buffer.from(inspect(error), 'utf8')]); + return Buffer.from(StringFromCharCode(kInspectedError) + inspect(error), 'utf8'); } function fromBuffer(error) { - return Buffer.from(error.buffer, - error.byteOffset + 1, - error.byteLength - 1); + return Buffer.from(TypedArrayPrototypeGetBuffer(error), + TypedArrayPrototypeGetByteOffset(error) + 1, + TypedArrayPrototypeGetByteLength(error) - 1); } let deserialize; From 5cc8701345e9f1f53536cb1a9153f5107cfae90b Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Fri, 12 May 2023 09:01:11 +0300 Subject: [PATCH 6/6] CR --- test/parallel/test-error-serdes.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-error-serdes.js b/test/parallel/test-error-serdes.js index 60d1dafb244567..3b61d3a4b9b34b 100644 --- a/test/parallel/test-error-serdes.js +++ b/test/parallel/test-error-serdes.js @@ -59,9 +59,15 @@ class ErrorWithThowingCause extends Error { } class ErrorWithCyclicCause extends Error { get cause() { - return new ErrorWithCyclicCause('err'); + return new ErrorWithCyclicCause(); } } +const errorWithCause = Object + .defineProperty(new Error('Error with cause'), 'cause', { get() { return { foo: 'bar' }; } }); +const errorWithThrowingCause = Object + .defineProperty(new Error('Error with cause'), 'cause', { get() { throw new Error('err'); } }); +const errorWithCyclicCause = Object + .defineProperty(new Error('Error with cause'), 'cause', { get() { return errorWithCyclicCause; } }); assert.strictEqual(cycle(new Error('Error with cause', { cause: 0 })).cause, 0); assert.strictEqual(cycle(new Error('Error with cause', { cause: -1 })).cause, -1); @@ -71,10 +77,9 @@ assert.strictEqual(cycle(new Error('Error with cause', { cause: undefined })).ca assert.strictEqual(Object.hasOwn(cycle(new Error('Error with cause', { cause: undefined })), 'cause'), true); assert.strictEqual(cycle(new Error('Error with cause', { cause: 'foo' })).cause, 'foo'); assert.deepStrictEqual(cycle(new Error('Error with cause', { cause: new Error('err') })).cause, new Error('err')); -assert.deepStrictEqual( - cycle(Object.defineProperty(new Error('Error with cause'), 'cause', { get() { return { foo: 'bar' }; } })).cause, - { foo: 'bar' } -); +assert.deepStrictEqual(cycle(errorWithCause).cause, { foo: 'bar' }); +assert.strictEqual(Object.hasOwn(cycle(errorWithThrowingCause), 'cause'), false); +assert.strictEqual(Object.hasOwn(cycle(errorWithCyclicCause), 'cause'), true); assert.deepStrictEqual(cycle(new ErrorWithCause('Error with cause')).cause, new Error('err')); assert.strictEqual(cycle(new ErrorWithThowingCause('Error with cause')).cause, undefined); assert.strictEqual(Object.hasOwn(cycle(new ErrorWithThowingCause('Error with cause')), 'cause'), false);