From 104dac79ccd0014549374fba5f0bcbb18353bd83 Mon Sep 17 00:00:00 2001
From: Antoine du Hamel <duhamelantoine1995@gmail.com>
Date: Thu, 4 Mar 2021 17:46:40 +0100
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/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>
---
 doc/api/fs.md                                 | 24 ++++++--
 lib/fs.js                                     | 16 ++---
 lib/internal/errors.js                        | 21 +++++++
 lib/internal/fs/read_file_context.js          |  3 +-
 lib/internal/http2/core.js                    |  3 +-
 lib/internal/per_context/primordials.js       |  1 +
 lib/internal/streams/destroy.js               |  9 ++-
 test/message/error_aggregateTwoErrors.js      | 13 ++++
 test/message/error_aggregateTwoErrors.out     | 13 ++++
 .../parallel/test-error-aggregateTwoErrors.js | 59 +++++++++++++++++++
 tools/doc/type-parser.js                      |  2 +-
 11 files changed, 147 insertions(+), 17 deletions(-)
 create mode 100644 test/message/error_aggregateTwoErrors.js
 create mode 100644 test/message/error_aggregateTwoErrors.out
 create mode 100644 test/parallel/test-error-aggregateTwoErrors.js

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 2a6ca89873ec83..5ffd9df27451a4 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -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.
diff --git a/lib/fs.js b/lib/fs.js
index b02ad976bdfa4e..743c330aacb38f 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -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) {
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index d8d565582ca970..34c168ce38ab91 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -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,
diff --git a/lib/internal/fs/read_file_context.js b/lib/internal/fs/read_file_context.js
index 5ec45882dbbdd5..4c50044368c7ec 100644
--- a/lib/internal/fs/read_file_context.js
+++ b/lib/internal/fs/read_file_context.js
@@ -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)
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index bb1cc9e6c56fa2..abcc460c423c78 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -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.
diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js
index a26e50005d6d43..311b3c324cfced 100644
--- a/lib/internal/per_context/primordials.js
+++ b/lib/internal/per_context/primordials.js
@@ -152,6 +152,7 @@ function copyPrototype(src, dest, prefix) {
 
 // Create copies of intrinsic objects
 [
+  'AggregateError',
   'Array',
   'ArrayBuffer',
   'BigInt',
diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js
index 76df2aa0a34666..467275c2edac35 100644
--- a/lib/internal/streams/destroy.js
+++ b/lib/internal/streams/destroy.js
@@ -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);
diff --git a/test/message/error_aggregateTwoErrors.js b/test/message/error_aggregateTwoErrors.js
new file mode 100644
index 00000000000000..a59a6b5788d4c2
--- /dev/null
+++ b/test/message/error_aggregateTwoErrors.js
@@ -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);
diff --git a/test/message/error_aggregateTwoErrors.out b/test/message/error_aggregateTwoErrors.out
new file mode 100644
index 00000000000000..d1dc13eacc303c
--- /dev/null
+++ b/test/message/error_aggregateTwoErrors.out
@@ -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'
+}
diff --git a/test/parallel/test-error-aggregateTwoErrors.js b/test/parallel/test-error-aggregateTwoErrors.js
new file mode 100644
index 00000000000000..89224efd6b161f
--- /dev/null
+++ b/test/parallel/test-error-aggregateTwoErrors.js
@@ -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]);
+}
diff --git a/tools/doc/type-parser.js b/tools/doc/type-parser.js
index c53628679594d4..0fc19c5c32ce0a 100644
--- a/tools/doc/type-parser.js
+++ b/tools/doc/type-parser.js
@@ -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',