fs: aggregate errors in fsPromises to avoid error swallowing

Add AggregateError support to fsPromises, instead of
swallowing errors if fs.close throws.

PR-URL: https://github.com/nodejs/node/pull/38259
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

lib/internal/fs/promises.js

@@ -10,6 +10,7 @@ const {
   PromisePrototypeFinally,
   PromisePrototypeThen,
   PromiseResolve,
+  PromiseReject,
   SafeArrayIterator,
   Symbol,
   Uint8Array,
@@ -33,6 +34,7 @@ const {
     ERR_METHOD_NOT_IMPLEMENTED,
   },
   AbortError,
+  aggregateTwoErrors,
 } = require('internal/errors');
 const { isArrayBufferView } = require('internal/util/types');
 const { rimrafPromises } = require('internal/fs/rimraf');
@@ -250,6 +252,19 @@ class FileHandle extends EventEmitterMixin(JSTransferable) {
   }
 }
 
+async function handleFdClose(fileOpPromise, closeFunc) {
+  return PromisePrototypeThen(
+    fileOpPromise,
+    (result) => PromisePrototypeThen(closeFunc(), () => result),
+    (opError) =>
+      PromisePrototypeThen(
+        closeFunc(),
+        () => PromiseReject(opError),
+        (closeError) => PromiseReject(aggregateTwoErrors(closeError, opError))
+      )
+  );
+}
+
 async function fsCall(fn, handle, ...args) {
   if (handle[kRefs] === undefined) {
     throw new ERR_INVALID_ARG_TYPE('filehandle', 'FileHandle', handle);
@@ -501,7 +516,7 @@ async function rename(oldPath, newPath) {
 
 async function truncate(path, len = 0) {
   const fd = await open(path, 'r+');
-  return PromisePrototypeFinally(ftruncate(fd, len), fd.close);
+  return handleFdClose(ftruncate(fd, len), fd.close);
 }
 
 async function ftruncate(handle, len = 0) {
@@ -632,7 +647,7 @@ async function lchmod(path, mode) {
     throw new ERR_METHOD_NOT_IMPLEMENTED('lchmod()');
 
   const fd = await open(path, O_WRONLY | O_SYMLINK);
-  return PromisePrototypeFinally(fchmod(fd, mode), fd.close);
+  return handleFdClose(fchmod(fd, mode), fd.close);
 }
 
 async function lchown(path, uid, gid) {
@@ -711,7 +726,7 @@ async function writeFile(path, data, options) {
   checkAborted(options.signal);
 
   const fd = await open(path, flag, options.mode);
-  return PromisePrototypeFinally(
+  return handleFdClose(
     writeFileHandle(fd, data, options.signal, options.encoding), fd.close);
 }
 
@@ -736,7 +751,7 @@ async function readFile(path, options) {
   checkAborted(options.signal);
 
   const fd = await open(path, flag, 0o666);
-  return PromisePrototypeFinally(readFileHandle(fd, options), fd.close);
+  return handleFdClose(readFileHandle(fd, options), fd.close);
 }
 
 module.exports = {

test/parallel/test-fs-promises-file-handle-aggregate-errors.js

@@ -0,0 +1,72 @@
+'use strict';
+// Flags: --expose-internals
+
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+
+// The following tests validate aggregate errors are thrown correctly
+// when both an operation and close throw.
+
+const path = require('path');
+const {
+  readFile,
+  writeFile,
+  truncate,
+  lchmod,
+} = require('fs/promises');
+const {
+  FileHandle,
+} = require('internal/fs/promises');
+
+const assert = require('assert');
+const originalFd = Object.getOwnPropertyDescriptor(FileHandle.prototype, 'fd');
+
+let count = 0;
+async function createFile() {
+  const filePath = path.join(tmpdir.path, `aggregate_errors_${++count}.txt`);
+  await writeFile(filePath, 'content');
+  return filePath;
+}
+
+async function checkAggregateError(op) {
+  try {
+    const filePath = await createFile();
+    Object.defineProperty(FileHandle.prototype, 'fd', {
+      get: function() {
+        // Close is set by using a setter,
+        // so it needs to be set on the instance.
+        const originalClose = this.close;
+        this.close = async () => {
+          // close the file
+          await originalClose.call(this);
+          const closeError = new Error('CLOSE_ERROR');
+          closeError.code = 456;
+          throw closeError;
+        };
+        const opError = new Error('INTERNAL_ERROR');
+        opError.code = 123;
+        throw opError;
+      }
+    });
+
+    await assert.rejects(op(filePath), common.mustCall((err) => {
+      assert.strictEqual(err.name, 'AggregateError');
+      assert.strictEqual(err.code, 123);
+      assert.strictEqual(err.errors.length, 2);
+      assert.strictEqual(err.errors[0].message, 'INTERNAL_ERROR');
+      assert.strictEqual(err.errors[1].message, 'CLOSE_ERROR');
+      return true;
+    }));
+  } finally {
+    Object.defineProperty(FileHandle.prototype, 'fd', originalFd);
+  }
+}
+(async function() {
+  tmpdir.refresh();
+  await checkAggregateError((filePath) => truncate(filePath));
+  await checkAggregateError((filePath) => readFile(filePath));
+  await checkAggregateError((filePath) => writeFile(filePath, '123'));
+  if (common.isOSX) {
+    await checkAggregateError((filePath) => lchmod(filePath, 0o777));
+  }
+})().then(common.mustCall());

test/parallel/test-fs-promises-file-handle-close-errors.js

@@ -0,0 +1,67 @@
+'use strict';
+// Flags: --expose-internals
+
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+
+// The following tests validate aggregate errors are thrown correctly
+// when both an operation and close throw.
+
+const path = require('path');
+const {
+  readFile,
+  writeFile,
+  truncate,
+  lchmod,
+} = require('fs/promises');
+const {
+  FileHandle,
+} = require('internal/fs/promises');
+
+const assert = require('assert');
+const originalFd = Object.getOwnPropertyDescriptor(FileHandle.prototype, 'fd');
+
+let count = 0;
+async function createFile() {
+  const filePath = path.join(tmpdir.path, `close_errors_${++count}.txt`);
+  await writeFile(filePath, 'content');
+  return filePath;
+}
+
+async function checkCloseError(op) {
+  try {
+    const filePath = await createFile();
+    Object.defineProperty(FileHandle.prototype, 'fd', {
+      get: function() {
+        // Close is set by using a setter,
+        // so it needs to be set on the instance.
+        const originalClose = this.close;
+        this.close = async () => {
+          // close the file
+          await originalClose.call(this);
+          const closeError = new Error('CLOSE_ERROR');
+          closeError.code = 456;
+          throw closeError;
+        };
+        return originalFd.get.call(this);
+      }
+    });
+
+    await assert.rejects(op(filePath), {
+      name: 'Error',
+      message: 'CLOSE_ERROR',
+      code: 456,
+    });
+  } finally {
+    Object.defineProperty(FileHandle.prototype, 'fd', originalFd);
+  }
+}
+(async function() {
+  tmpdir.refresh();
+  await checkCloseError((filePath) => truncate(filePath));
+  await checkCloseError((filePath) => readFile(filePath));
+  await checkCloseError((filePath) => writeFile(filePath, '123'));
+  if (common.isOSX) {
+    await checkCloseError((filePath) => lchmod(filePath, 0o777));
+  }
+})().then(common.mustCall());

test/parallel/test-fs-promises-file-handle-op-errors.js

@@ -0,0 +1,61 @@
+'use strict';
+// Flags: --expose-internals
+
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+
+// The following tests validate aggregate errors are thrown correctly
+// when both an operation and close throw.
+
+const path = require('path');
+const {
+  readFile,
+  writeFile,
+  truncate,
+  lchmod,
+} = require('fs/promises');
+const {
+  FileHandle,
+} = require('internal/fs/promises');
+
+const assert = require('assert');
+const originalFd = Object.getOwnPropertyDescriptor(FileHandle.prototype, 'fd');
+
+let count = 0;
+async function createFile() {
+  const filePath = path.join(tmpdir.path, `op_errors_${++count}.txt`);
+  await writeFile(filePath, 'content');
+  return filePath;
+}
+
+async function checkOperationError(op) {
+  try {
+    const filePath = await createFile();
+    Object.defineProperty(FileHandle.prototype, 'fd', {
+      get: function() {
+        // Verify that close is called when an error is thrown
+        this.close = common.mustCall(this.close);
+        const opError = new Error('INTERNAL_ERROR');
+        opError.code = 123;
+        throw opError;
+      }
+    });
+
+    await assert.rejects(op(filePath), {
+      name: 'Error',
+      message: 'INTERNAL_ERROR',
+      code: 123,
+    });
+  } finally {
+    Object.defineProperty(FileHandle.prototype, 'fd', originalFd);
+  }
+}
+(async function() {
+  tmpdir.refresh();
+  await checkOperationError((filePath) => truncate(filePath));
+  await checkOperationError((filePath) => readFile(filePath));
+  await checkOperationError((filePath) => writeFile(filePath, '123'));
+  if (common.isOSX) {
+    await checkOperationError((filePath) => lchmod(filePath, 0o777));
+  }
+})().then(common.mustCall());