lib: aggregate errors to avoid error swallowing · nodejs/node@104dac7 · GitHub
Skip to content

Commit 104dac7

Browse files
aduh95aduh95
committed
lib: aggregate errors to avoid error swallowing
Uses `AggregateError` if there are more than one error with the message of the outer error to preserve the current behaviour, or returns the logical OR comparison of the two parameters. PR-URL: #37460 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>
1 parent 0ddd75b commit 104dac7

11 files changed

Lines changed: 147 additions & 17 deletions

File tree

‎doc/api/fs.md‎

Lines changed: 20 additions & 4 deletions

‎lib/fs.js‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ const { isArrayBufferView } = require('internal/util/types');
7474
const binding = internalBinding('fs');
7575
const { Buffer } = require('buffer');
7676
const {
77+
aggregateTwoErrors,
7778
codes: {
7879
ERR_FS_FILE_TOO_LARGE,
7980
ERR_INVALID_ARG_VALUE,
@@ -826,7 +827,7 @@ function truncate(path, len, callback) {
826827
const req = new FSReqCallback();
827828
req.oncomplete = function oncomplete(er) {
828829
fs.close(fd, (er2) => {
829-
callback(er || er2);
830+
callback(aggregateTwoErrors(er2, er));
830831
});
831832
};
832833
binding.ftruncate(fd, len, req);
@@ -1296,7 +1297,7 @@ function lchmod(path, mode, callback) {
12961297
// but still try to close, and report closing errors if they occur.
12971298
fs.fchmod(fd, mode, (err) => {
12981299
fs.close(fd, (err2) => {
1299-
callback(err || err2);
1300+
callback(aggregateTwoErrors(err2, err));
13001301
});
13011302
});
13021303
});
@@ -1461,11 +1462,12 @@ function lutimesSync(path, atime, mtime) {
14611462

14621463
function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
14631464
if (signal?.aborted) {
1465+
const abortError = new AbortError();
14641466
if (isUserFd) {
1465-
callback(new AbortError());
1467+
callback(abortError);
14661468
} else {
1467-
fs.close(fd, function() {
1468-
callback(new AbortError());
1469+
fs.close(fd, (err) => {
1470+
callback(aggregateTwoErrors(err, abortError));
14691471
});
14701472
}
14711473
return;
@@ -1476,8 +1478,8 @@ function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
14761478
if (isUserFd) {
14771479
callback(writeErr);
14781480
} else {
1479-
fs.close(fd, function close() {
1480-
callback(writeErr);
1481+
fs.close(fd, (err) => {
1482+
callback(aggregateTwoErrors(err, writeErr));
14811483
});
14821484
}
14831485
} else if (written === length) {

‎lib/internal/errors.js‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
// message may change, the code should not.
1212

1313
const {
14+
AggregateError,
1415
ArrayFrom,
1516
ArrayIsArray,
1617
ArrayPrototypeIncludes,
@@ -36,6 +37,7 @@ const {
3637
RangeError,
3738
ReflectApply,
3839
RegExpPrototypeTest,
40+
SafeArrayIterator,
3941
SafeMap,
4042
SafeWeakMap,
4143
String,
@@ -136,6 +138,24 @@ const maybeOverridePrepareStackTrace = (globalThis, error, trace) => {
136138
return kNoOverride;
137139
};
138140

141+
const aggregateTwoErrors = hideStackFrames((innerError, outerError) => {
142+
if (innerError && outerError) {
143+
if (ArrayIsArray(outerError.errors)) {
144+
// If `outerError` is already an `AggregateError`.
145+
ArrayPrototypePush(outerError.errors, innerError);
146+
return outerError;
147+
}
148+
// eslint-disable-next-line no-restricted-syntax
149+
const err = new AggregateError(new SafeArrayIterator([
150+
outerError,
151+
innerError,
152+
]), outerError.message);
153+
err.code = outerError.code;
154+
return err;
155+
}
156+
return innerError || outerError;
157+
});
158+
139159
// Lazily loaded
140160
let util;
141161
let assert;
@@ -752,6 +772,7 @@ class AbortError extends Error {
752772
}
753773
module.exports = {
754774
addCodeToName, // Exported for NghttpError
775+
aggregateTwoErrors,
755776
codes,
756777
dnsException,
757778
errnoException,

‎lib/internal/fs/read_file_context.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ const { FSReqCallback, close, read } = internalBinding('fs');
1212

1313
const {
1414
AbortError,
15+
aggregateTwoErrors,
1516
} = require('internal/errors');
1617

1718
// 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) {
5051
let buffer = null;
5152

5253
if (context.err || err)
53-
return callback(context.err || err);
54+
return callback(aggregateTwoErrors(err, context.err));
5455

5556
try {
5657
if (context.size === 0)

‎lib/internal/http2/core.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ const {
6565
},
6666
} = require('internal/async_hooks');
6767
const {
68+
aggregateTwoErrors,
6869
codes: {
6970
ERR_HTTP2_ALTSVC_INVALID_ORIGIN,
7071
ERR_HTTP2_ALTSVC_LENGTH,
@@ -2077,7 +2078,7 @@ class Http2Stream extends Duplex {
20772078
let endCheckCallbackErr;
20782079
const done = () => {
20792080
if (waitingForEndCheck || waitingForWriteCallback) return;
2080-
const err = writeCallbackErr || endCheckCallbackErr;
2081+
const err = aggregateTwoErrors(endCheckCallbackErr, writeCallbackErr);
20812082
// writeGeneric does not destroy on error and
20822083
// we cannot enable autoDestroy,
20832084
// so make sure to destroy on error.

‎lib/internal/per_context/primordials.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ function copyPrototype(src, dest, prefix) {
152152

153153
// Create copies of intrinsic objects
154154
[
155+
'AggregateError',
155156
'Array',
156157
'ArrayBuffer',
157158
'BigInt',

‎lib/internal/streams/destroy.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
'use strict';
22

33
const {
4-
ERR_MULTIPLE_CALLBACK
5-
} = require('internal/errors').codes;
4+
aggregateTwoErrors,
5+
codes: {
6+
ERR_MULTIPLE_CALLBACK,
7+
},
8+
} = require('internal/errors');
69
const {
710
FunctionPrototypeCall,
811
Symbol,
@@ -56,7 +59,7 @@ function destroy(err, cb) {
5659
// If still constructing then defer calling _destroy.
5760
if (!s.constructed) {
5861
this.once(kDestroy, function(er) {
59-
_destroy(this, err || er, cb);
62+
_destroy(this, aggregateTwoErrors(er, err), cb);
6063
});
6164
} else {
6265
_destroy(this, err, cb);

‎test/message/error_aggregateTwoErrors.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// Flags: --expose-internals
2+
'use strict';
3+
4+
require('../common');
5+
const { aggregateTwoErrors } = require('internal/errors');
6+
7+
const originalError = new Error('original');
8+
const err = new Error('second error');
9+
10+
originalError.code = 'ERR0';
11+
err.code = 'ERR1';
12+
13+
throw aggregateTwoErrors(err, originalError);

‎test/message/error_aggregateTwoErrors.out‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
*error_aggregateTwoErrors.js:*
2+
throw aggregateTwoErrors(err, originalError);
3+
^
4+
AggregateError: original
5+
at Object.<anonymous> (*test*message*error_aggregateTwoErrors.js:*:*)
6+
at Module._compile (node:internal/modules/cjs/loader:*:*)
7+
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*:*)
8+
at Module.load (node:internal/modules/cjs/loader:*:*)
9+
at Function.Module._load (node:internal/modules/cjs/loader:*:*)
10+
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:*:*)
11+
at node:internal/main/run_main_module:*:* {
12+
code: 'ERR0'
13+
}

‎test/parallel/test-error-aggregateTwoErrors.js‎

Lines changed: 59 additions & 0 deletions

0 commit comments

Comments
 (0)