Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: add chainErrors API to avoid error swallowing #37460

Merged
merged 1 commit into from
Mar 19, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
@@ -2336,6 +2336,10 @@ error `UV_ENOSYS`.
<!-- YAML
deprecated: v0.4.7
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37460
description: The error returned may be an `AggregateError` if more than one
error is returned.
- version: v10.0.0
pr-url: https://github.com/nodejs/node/pull/12562
description: The `callback` parameter is no longer optional. Not passing
@@ -2349,7 +2353,7 @@ changes:
* `path` {string|Buffer|URL}
* `mode` {integer}
* `callback` {Function}
* `err` {Error}
* `err` {Error|AggregateError}

Changes the permissions on a symbolic link. No arguments other than a possible
exception are given to the completion callback.
@@ -2812,6 +2816,10 @@ If `options.withFileTypes` is set to `true`, the `files` array will contain
<!-- YAML
added: v0.1.29
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37460
description: The error returned may be an `AggregateError` if more than one
error is returned.
- version: v15.2.0
pr-url: https://github.com/nodejs/node/pull/35911
description: The options argument may include an AbortSignal to abort an
@@ -2843,7 +2851,7 @@ changes:
* `flag` {string} See [support of file system `flags`][]. **Default:** `'r'`.
* `signal` {AbortSignal} allows aborting an in-progress readFile
* `callback` {Function}
* `err` {Error}
* `err` {Error|AggregateError}
* `data` {string|Buffer}

Asynchronously reads the entire contents of a file.
@@ -3390,6 +3398,10 @@ example/
<!-- YAML
added: v0.8.6
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37460
description: The error returned may be an `AggregateError` if more than one
error is returned.
- version: v10.0.0
pr-url: https://github.com/nodejs/node/pull/12562
description: The `callback` parameter is no longer optional. Not passing
@@ -3403,7 +3415,7 @@ changes:
* `path` {string|Buffer|URL}
* `len` {integer} **Default:** `0`
* `callback` {Function}
* `err` {Error}
* `err` {Error|AggregateError}

Truncates the file. No arguments other than a possible exception are
given to the completion callback. A file descriptor can also be passed as the
@@ -3843,6 +3855,10 @@ details.
<!-- YAML
added: v0.1.29
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37460
description: The error returned may be an `AggregateError` if more than one
error is returned.
- version: v15.2.0
pr-url: https://github.com/nodejs/node/pull/35993
description: The options argument may include an AbortSignal to abort an
@@ -3883,7 +3899,7 @@ changes:
* `flag` {string} See [support of file system `flags`][]. **Default:** `'w'`.
* `signal` {AbortSignal} allows aborting an in-progress writeFile
* `callback` {Function}
* `err` {Error}
* `err` {Error|AggregateError}

When `file` is a filename, asynchronously writes data to the file, replacing the
file if it already exists. `data` can be a string or a buffer.
16 changes: 9 additions & 7 deletions lib/fs.js
Original file line number Diff line number Diff line change
@@ -74,6 +74,7 @@ const { isArrayBufferView } = require('internal/util/types');
const binding = internalBinding('fs');
const { Buffer } = require('buffer');
const {
aggregateTwoErrors,
codes: {
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_VALUE,
@@ -826,7 +827,7 @@ function truncate(path, len, callback) {
const req = new FSReqCallback();
req.oncomplete = function oncomplete(er) {
fs.close(fd, (er2) => {
callback(er || er2);
callback(aggregateTwoErrors(er2, er));
});
};
binding.ftruncate(fd, len, req);
@@ -1296,7 +1297,7 @@ function lchmod(path, mode, callback) {
// but still try to close, and report closing errors if they occur.
fs.fchmod(fd, mode, (err) => {
fs.close(fd, (err2) => {
callback(err || err2);
callback(aggregateTwoErrors(err2, err));
});
});
});
@@ -1461,11 +1462,12 @@ function lutimesSync(path, atime, mtime) {

function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
if (signal?.aborted) {
const abortError = new AbortError();
if (isUserFd) {
callback(new AbortError());
callback(abortError);
} else {
fs.close(fd, function() {
callback(new AbortError());
fs.close(fd, (err) => {
callback(aggregateTwoErrors(err, abortError));
});
}
return;
@@ -1476,8 +1478,8 @@ function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
if (isUserFd) {
callback(writeErr);
} else {
fs.close(fd, function close() {
callback(writeErr);
fs.close(fd, (err) => {
callback(aggregateTwoErrors(err, writeErr));
});
}
} else if (written === length) {
21 changes: 21 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@
// message may change, the code should not.

const {
AggregateError,
ArrayFrom,
ArrayIsArray,
ArrayPrototypeIncludes,
@@ -36,6 +37,7 @@ const {
RangeError,
ReflectApply,
RegExpPrototypeTest,
SafeArrayIterator,
SafeMap,
SafeWeakMap,
String,
@@ -136,6 +138,24 @@ const maybeOverridePrepareStackTrace = (globalThis, error, trace) => {
return kNoOverride;
};

const aggregateTwoErrors = hideStackFrames((innerError, outerError) => {
if (innerError && outerError) {
if (ArrayIsArray(outerError.errors)) {
// If `outerError` is already an `AggregateError`.
ArrayPrototypePush(outerError.errors, innerError);
return outerError;
}
// eslint-disable-next-line no-restricted-syntax
const err = new AggregateError(new SafeArrayIterator([
outerError,
innerError,
]), outerError.message);
err.code = outerError.code;
return err;
}
return innerError || outerError;
});

// Lazily loaded
let util;
let assert;
@@ -752,6 +772,7 @@ class AbortError extends Error {
}
module.exports = {
addCodeToName, // Exported for NghttpError
aggregateTwoErrors,
codes,
dnsException,
errnoException,
3 changes: 2 additions & 1 deletion lib/internal/fs/read_file_context.js
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ const { FSReqCallback, close, read } = internalBinding('fs');

const {
AbortError,
aggregateTwoErrors,
} = require('internal/errors');

// Use 64kb in case the file type is not a regular file and thus do not know the
@@ -50,7 +51,7 @@ function readFileAfterClose(err) {
let buffer = null;

if (context.err || err)
return callback(context.err || err);
return callback(aggregateTwoErrors(err, context.err));

try {
if (context.size === 0)
3 changes: 2 additions & 1 deletion lib/internal/http2/core.js
Original file line number Diff line number Diff line change
@@ -65,6 +65,7 @@ const {
},
} = require('internal/async_hooks');
const {
aggregateTwoErrors,
codes: {
ERR_HTTP2_ALTSVC_INVALID_ORIGIN,
ERR_HTTP2_ALTSVC_LENGTH,
@@ -2077,7 +2078,7 @@ class Http2Stream extends Duplex {
let endCheckCallbackErr;
const done = () => {
if (waitingForEndCheck || waitingForWriteCallback) return;
const err = writeCallbackErr || endCheckCallbackErr;
const err = aggregateTwoErrors(endCheckCallbackErr, writeCallbackErr);
// writeGeneric does not destroy on error and
// we cannot enable autoDestroy,
// so make sure to destroy on error.
1 change: 1 addition & 0 deletions lib/internal/per_context/primordials.js
Original file line number Diff line number Diff line change
@@ -152,6 +152,7 @@ function copyPrototype(src, dest, prefix) {

// Create copies of intrinsic objects
[
'AggregateError',
'Array',
'ArrayBuffer',
'BigInt',
9 changes: 6 additions & 3 deletions lib/internal/streams/destroy.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
'use strict';

const {
ERR_MULTIPLE_CALLBACK
} = require('internal/errors').codes;
aggregateTwoErrors,
codes: {
ERR_MULTIPLE_CALLBACK,
},
} = require('internal/errors');
const {
FunctionPrototypeCall,
Symbol,
@@ -56,7 +59,7 @@ function destroy(err, cb) {
// If still constructing then defer calling _destroy.
if (!s.constructed) {
this.once(kDestroy, function(er) {
_destroy(this, err || er, cb);
_destroy(this, aggregateTwoErrors(er, err), cb);
});
} else {
_destroy(this, err, cb);
13 changes: 13 additions & 0 deletions test/message/error_aggregateTwoErrors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Flags: --expose-internals
'use strict';

require('../common');
const { aggregateTwoErrors } = require('internal/errors');

const originalError = new Error('original');
const err = new Error('second error');

originalError.code = 'ERR0';
err.code = 'ERR1';

throw aggregateTwoErrors(err, originalError);
13 changes: 13 additions & 0 deletions test/message/error_aggregateTwoErrors.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
*error_aggregateTwoErrors.js:*
throw aggregateTwoErrors(err, originalError);
^
AggregateError: original
at Object.<anonymous> (*test*message*error_aggregateTwoErrors.js:*:*)
at Module._compile (node:internal/modules/cjs/loader:*:*)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*:*)
at Module.load (node:internal/modules/cjs/loader:*:*)
at Function.Module._load (node:internal/modules/cjs/loader:*:*)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:*:*)
at node:internal/main/run_main_module:*:* {
code: 'ERR0'
}
59 changes: 59 additions & 0 deletions test/parallel/test-error-aggregateTwoErrors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// Flags: --expose-internals
'use strict';

require('../common');
const assert = require('assert');
const { aggregateTwoErrors } = require('internal/errors');

assert.strictEqual(aggregateTwoErrors(null, null), null);

{
const err = new Error();
assert.strictEqual(aggregateTwoErrors(null, err), err);
}

{
const err = new Error();
assert.strictEqual(aggregateTwoErrors(err, null), err);
}

{
const err0 = new Error('original');
const err1 = new Error('second error');

err0.code = 'ERR0';
err1.code = 'ERR1';

const chainedError = aggregateTwoErrors(err1, err0);
assert.strictEqual(chainedError.message, err0.message);
assert.strictEqual(chainedError.code, err0.code);
assert.deepStrictEqual(chainedError.errors, [err0, err1]);
}

{
const err0 = new Error('original');
const err1 = new Error('second error');
const err2 = new Error('third error');

err0.code = 'ERR0';
err1.code = 'ERR1';
err2.code = 'ERR2';

const chainedError = aggregateTwoErrors(err2, aggregateTwoErrors(err1, err0));
assert.strictEqual(chainedError.message, err0.message);
assert.strictEqual(chainedError.code, err0.code);
assert.deepStrictEqual(chainedError.errors, [err0, err1, err2]);
}

{
const err0 = new Error('original');
const err1 = new Error('second error');

err0.code = 'ERR0';
err1.code = 'ERR1';

const chainedError = aggregateTwoErrors(null, aggregateTwoErrors(err1, err0));
assert.strictEqual(chainedError.message, err0.message);
assert.strictEqual(chainedError.code, err0.code);
assert.deepStrictEqual(chainedError.errors, [err0, err1]);
}
2 changes: 1 addition & 1 deletion tools/doc/type-parser.js
Original file line number Diff line number Diff line change
@@ -15,7 +15,7 @@ const jsPrimitives = {

const jsGlobalObjectsUrl = `${jsDocPrefix}Reference/Global_Objects/`;
const jsGlobalTypes = [
'Array', 'ArrayBuffer', 'DataView', 'Date', 'Error',
'AggregateError', 'Array', 'ArrayBuffer', 'DataView', 'Date', 'Error',
'EvalError', 'Function', 'Map', 'Object', 'Promise', 'RangeError',
'ReferenceError', 'RegExp', 'Set', 'SharedArrayBuffer', 'SyntaxError',
'TypeError', 'TypedArray', 'URIError', 'Uint8Array',