Skip to content

Commit

Permalink
worker: improve error (de)serialization
Browse files Browse the repository at this point in the history
Rather than passing errors using some sort of string representation,
do a best effort for faithful serialization/deserialization of
uncaught exception objects.

PR-URL: #20876
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
  • Loading branch information
addaleax authored and targos committed Jun 13, 2018
1 parent ecba1c5 commit 9ad42b7
Show file tree
Hide file tree
Showing 6 changed files with 171 additions and 16 deletions.
121 changes: 121 additions & 0 deletions lib/internal/error-serdes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
'use strict';

const Buffer = require('buffer').Buffer;
const { serialize, deserialize } = require('v8');
const { SafeSet } = require('internal/safe_globals');

const kSerializedError = 0;
const kSerializedObject = 1;
const kInspectedError = 2;

const GetPrototypeOf = Object.getPrototypeOf;
const GetOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;
const GetOwnPropertyNames = Object.getOwnPropertyNames;
const DefineProperty = Object.defineProperty;
const Assign = Object.assign;
const ObjectPrototypeToString =
Function.prototype.call.bind(Object.prototype.toString);
const ForEach = Function.prototype.call.bind(Array.prototype.forEach);
const Call = Function.prototype.call.bind(Function.prototype.call);

const errors = {
Error, TypeError, RangeError, URIError, SyntaxError, ReferenceError, EvalError
};
const errorConstructorNames = new SafeSet(Object.keys(errors));

function TryGetAllProperties(object, target = object) {
const all = Object.create(null);
if (object === null)
return all;
Assign(all, TryGetAllProperties(GetPrototypeOf(object), target));
const keys = GetOwnPropertyNames(object);
ForEach(keys, (key) => {
const descriptor = GetOwnPropertyDescriptor(object, key);
const getter = descriptor.get;
if (getter && key !== '__proto__') {
try {
descriptor.value = Call(getter, target);
} catch {}
}
if ('value' in descriptor && typeof descriptor.value !== 'function') {
delete descriptor.get;
delete descriptor.set;
all[key] = descriptor;
}
});
return all;
}

function GetConstructors(object) {
const constructors = [];

for (var current = object;
current !== null;
current = GetPrototypeOf(current)) {
const desc = GetOwnPropertyDescriptor(current, 'constructor');
if (desc && desc.value) {
DefineProperty(constructors, constructors.length, {
value: desc.value, enumerable: true
});
}
}

return constructors;
}

function GetName(object) {
const desc = GetOwnPropertyDescriptor(object, 'name');
return desc && desc.value;
}

let util;
function lazyUtil() {
if (!util)
util = require('util');
return util;
}

function serializeError(error) {
try {
if (typeof error === 'object' &&
ObjectPrototypeToString(error) === '[object Error]') {
const constructors = GetConstructors(error);
for (var i = constructors.length - 1; i >= 0; i--) {
const name = GetName(constructors[i]);
if (errorConstructorNames.has(name)) {
try { error.stack; } catch {}
const serialized = serialize({
constructor: name,
properties: TryGetAllProperties(error)
});
return Buffer.concat([Buffer.from([kSerializedError]), serialized]);
}
}
}
} catch {}
try {
const serialized = serialize(error);
return Buffer.concat([Buffer.from([kSerializedObject]), serialized]);
} catch {}
return Buffer.concat([Buffer.from([kInspectedError]),
Buffer.from(lazyUtil().inspect(error), 'utf8')]);
}

function deserializeError(error) {
switch (error[0]) {
case kSerializedError:
const { constructor, properties } = deserialize(error.subarray(1));
const ctor = errors[constructor];
return Object.create(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');
}
require('assert').fail('This should not happen');
}

module.exports = { serializeError, deserializeError };
13 changes: 1 addition & 12 deletions lib/internal/worker.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use strict';

const Buffer = require('buffer').Buffer;
const EventEmitter = require('events');
const assert = require('assert');
const path = require('path');
Expand All @@ -17,6 +16,7 @@ const { internalBinding } = require('internal/bootstrap/loaders');
const { MessagePort, MessageChannel } = internalBinding('messaging');
const { handle_onclose } = internalBinding('symbols');
const { clearAsyncIdStack } = require('internal/async_hooks');
const { serializeError, deserializeError } = require('internal/error-serdes');

util.inherits(MessagePort, EventEmitter);

Expand Down Expand Up @@ -453,17 +453,6 @@ function setupChild(evalScript) {
}
}

// TODO(addaleax): These can be improved a lot.
function serializeError(error) {
return Buffer.from(util.inspect(error), 'utf8');
}

function deserializeError(error) {
return Buffer.from(error.buffer,
error.byteOffset,
error.byteLength).toString('utf8');
}

function pipeWithoutWarning(source, dest) {
const sourceMaxListeners = source._maxListeners;
const destMaxListeners = dest._maxListeners;
Expand Down
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@
'lib/internal/constants.js',
'lib/internal/encoding.js',
'lib/internal/errors.js',
'lib/internal/error-serdes.js',
'lib/internal/fixed_queue.js',
'lib/internal/freelist.js',
'lib/internal/fs/promises.js',
Expand Down
46 changes: 46 additions & 0 deletions test/parallel/test-error-serdes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Flags: --expose-internals
'use strict';
require('../common');
const assert = require('assert');
const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes;
const { serializeError, deserializeError } = require('internal/error-serdes');

function cycle(err) {
return deserializeError(serializeError(err));
}

assert.strictEqual(cycle(0), 0);
assert.strictEqual(cycle(-1), -1);
assert.strictEqual(cycle(1.4), 1.4);
assert.strictEqual(cycle(null), null);
assert.strictEqual(cycle(undefined), undefined);
assert.strictEqual(cycle('foo'), 'foo');

{
const err = cycle(new Error('foo'));
assert(err instanceof Error);
assert.strictEqual(err.name, 'Error');
assert.strictEqual(err.message, 'foo');
assert(/^Error: foo\n/.test(err.stack));
}

assert.strictEqual(cycle(new RangeError('foo')).name, 'RangeError');
assert.strictEqual(cycle(new TypeError('foo')).name, 'TypeError');
assert.strictEqual(cycle(new ReferenceError('foo')).name, 'ReferenceError');
assert.strictEqual(cycle(new URIError('foo')).name, 'URIError');
assert.strictEqual(cycle(new EvalError('foo')).name, 'EvalError');
assert.strictEqual(cycle(new SyntaxError('foo')).name, 'SyntaxError');

class SubError extends Error {}

assert.strictEqual(cycle(new SubError('foo')).name, 'Error');

assert.deepStrictEqual(cycle({ message: 'foo' }), { message: 'foo' });
assert.strictEqual(cycle(Function), '[Function: Function]');

{
const err = new ERR_INVALID_ARG_TYPE('object', 'Object', 42);
assert(/^TypeError \[ERR_INVALID_ARG_TYPE\]:/.test(err));
assert.strictEqual(err.name, 'TypeError [ERR_INVALID_ARG_TYPE]');
assert.strictEqual(err.code, 'ERR_INVALID_ARG_TYPE');
}
3 changes: 1 addition & 2 deletions test/parallel/test-worker-uncaught-exception-async.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ if (!process.env.HAS_STARTED_WORKER) {
const w = new Worker(__filename);
w.on('message', common.mustNotCall());
w.on('error', common.mustCall((err) => {
// TODO(addaleax): be more specific here
assert(/foo/.test(err));
assert(/^Error: foo$/.test(err));
}));
} else {
setImmediate(() => {
Expand Down
3 changes: 1 addition & 2 deletions test/parallel/test-worker-uncaught-exception.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ if (!process.env.HAS_STARTED_WORKER) {
const w = new Worker(__filename);
w.on('message', common.mustNotCall());
w.on('error', common.mustCall((err) => {
// TODO(addaleax): be more specific here
assert(/foo/.test(err));
assert(/^Error: foo$/.test(err));
}));
} else {
throw new Error('foo');
Expand Down

0 comments on commit 9ad42b7

Please sign in to comment.