fs: aggregate errors in fsPromises to avoid error swallowing

Linkgoron · jasnell · commit f27b7cf95c18 · 2021-04-26T11:53:25.000-07:00
Add AggregateError support to fsPromises, instead of swallowing errors if fs.close throws. PR-URL: #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>
diff --git a/lib/internal/fs/promises.js b/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 = {
diff --git a/test/parallel/test-fs-promises-file-handle-aggregate-errors.js b/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());
diff --git a/test/parallel/test-fs-promises-file-handle-close-errors.js b/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());
diff --git a/test/parallel/test-fs-promises-file-handle-op-errors.js b/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());

-Original file line number
+Diff line change
  PromisePrototypeFinally,
  PromisePrototypeThen,
  PromiseResolve,
 + PromiseReject,
  SafeArrayIterator,
  Symbol,
  Uint8Array,
 ERR_METHOD_NOT_IMPLEMENTED,
 },
  AbortError,
 + aggregateTwoErrors,
 }=require('internal/errors');
 const{ isArrayBufferView }=require('internal/util/types');
 const{ rimrafPromises }=require('internal/fs/rimraf');
+}
+}
 +asyncfunctionhandleFdClose(fileOpPromise,closeFunc){
 +returnPromisePrototypeThen(
 +fileOpPromise,
 +(result)=>PromisePrototypeThen(closeFunc(),()=>result),
 +(opError)=>
 +PromisePrototypeThen(
 +closeFunc(),
 +()=>PromiseReject(opError),
 +(closeError)=>PromiseReject(aggregateTwoErrors(closeError,opError))
 +)
 +);
 +}
++
 asyncfunctionfsCall(fn,handle, ...args){
 if(handle[kRefs]===undefined){
 thrownewERR_INVALID_ARG_TYPE('filehandle','FileHandle',handle);
 asyncfunctiontruncate(path,len=0){
 constfd=awaitopen(path,'r+');
 -returnPromisePrototypeFinally(ftruncate(fd,len),fd.close);
 +returnhandleFdClose(ftruncate(fd,len),fd.close);
+}
 asyncfunctionftruncate(handle,len=0){
 thrownewERR_METHOD_NOT_IMPLEMENTED('lchmod()');
 constfd=awaitopen(path,O_WRONLY|O_SYMLINK);
 -returnPromisePrototypeFinally(fchmod(fd,mode),fd.close);
 +returnhandleFdClose(fchmod(fd,mode),fd.close);
+}
 asyncfunctionlchown(path,uid,gid){
 checkAborted(options.signal);
 constfd=awaitopen(path,flag,options.mode);
 -returnPromisePrototypeFinally(
 +returnhandleFdClose(
 writeFileHandle(fd,data,options.signal,options.encoding),fd.close);
+}
 checkAborted(options.signal);
 constfd=awaitopen(path,flag,0o666);
 -returnPromisePrototypeFinally(readFileHandle(fd,options),fd.close);
 +returnhandleFdClose(readFileHandle(fd,options),fd.close);
+}
 module.exports={
-Original file line number
+Diff line change
@@ @@ -0,0 +1,72 @@ @@
 +'use strict';
 +// Flags: --expose-internals
++
 +constcommon=require('../common');
 +consttmpdir=require('../common/tmpdir');
++
 +// The following tests validate aggregate errors are thrown correctly
 +// when both an operation and close throw.
++
 +constpath=require('path');
 +const{
 + readFile,
 + writeFile,
 + truncate,
 + lchmod,
 +}=require('fs/promises');
 +const{
 + FileHandle,
 +}=require('internal/fs/promises');
++
 +constassert=require('assert');
 +constoriginalFd=Object.getOwnPropertyDescriptor(FileHandle.prototype,'fd');
++
 +letcount=0;
 +asyncfunctioncreateFile(){
 +constfilePath=path.join(tmpdir.path,`aggregate_errors_${++count}.txt`);
 +awaitwriteFile(filePath,'content');
 +returnfilePath;
 +}
++
 +asyncfunctioncheckAggregateError(op){
 +try{
 +constfilePath=awaitcreateFile();
 +Object.defineProperty(FileHandle.prototype,'fd',{
 +get: function(){
 +// Close is set by using a setter,
 +// so it needs to be set on the instance.
 +constoriginalClose=this.close;
 +this.close=async()=>{
 +// close the file
 +awaitoriginalClose.call(this);
 +constcloseError=newError('CLOSE_ERROR');
 +closeError.code=456;
 +throwcloseError;
 +};
 +constopError=newError('INTERNAL_ERROR');
 +opError.code=123;
 +throwopError;
 +}
 +});
++
 +awaitassert.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');
 +returntrue;
 +}));
 +}finally{
 +Object.defineProperty(FileHandle.prototype,'fd',originalFd);
 +}
 +}
 +(asyncfunction(){
 +tmpdir.refresh();
 +awaitcheckAggregateError((filePath)=>truncate(filePath));
 +awaitcheckAggregateError((filePath)=>readFile(filePath));
 +awaitcheckAggregateError((filePath)=>writeFile(filePath,'123'));
 +if(common.isOSX){
 +awaitcheckAggregateError((filePath)=>lchmod(filePath,0o777));
 +}
 +})().then(common.mustCall());