From b35dfd1aee165fd9d1324a57034cd60439a6b538 Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Fri, 16 Apr 2021 15:50:36 +0300 Subject: [PATCH 1/3] fs: aggregate errors in fsPromises to avoid error swallowing Add AggregateError support to fsPromises, instead of swallowing errors if fs.close throws. --- lib/internal/fs/promises.js | 23 ++++-- ...s-promises-file-handle-aggregate-errors.js | 72 +++++++++++++++++++ ...st-fs-promises-file-handle-close-errors.js | 68 ++++++++++++++++++ .../test-fs-promises-file-handle-op-errors.js | 66 +++++++++++++++++ 4 files changed, 225 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-fs-promises-file-handle-aggregate-errors.js create mode 100644 test/parallel/test-fs-promises-file-handle-close-errors.js create mode 100644 test/parallel/test-fs-promises-file-handle-op-errors.js diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 6ec895f1a0c556..75052ab4c4f20f 100644 --- 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 new file mode 100644 index 00000000000000..35c405cc3a90e8 --- /dev/null +++ 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 fs = require('fs'); +const path = require('path'); +const { + readFile, + writeFile, + truncate, + lchmod, +} = 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 op(filePath).catch(common.mustCall((err) => { + assert.strictEqual(err.constructor.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'); + })); + } 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 new file mode 100644 index 00000000000000..d43f811b7c6fc0 --- /dev/null +++ b/test/parallel/test-fs-promises-file-handle-close-errors.js @@ -0,0 +1,68 @@ +'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 fs = require('fs'); +const path = require('path'); +const { + readFile, + writeFile, + truncate, + lchmod, +} = 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 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; + }; + return originalFd.get.call(this); + } + }); + + await op(filePath).catch(common.mustCall((err) => { + assert.strictEqual(err.constructor.name, 'Error'); + assert.strictEqual(err.message, 'CLOSE_ERROR'); + assert.strictEqual(err.code, 456); + })); + } 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-op-errors.js b/test/parallel/test-fs-promises-file-handle-op-errors.js new file mode 100644 index 00000000000000..dfd378f62d5670 --- /dev/null +++ b/test/parallel/test-fs-promises-file-handle-op-errors.js @@ -0,0 +1,66 @@ +'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 fs = require('fs'); +const path = require('path'); +const { + readFile, + writeFile, + truncate, + lchmod, +} = 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 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 = common.mustCall(function(...args) { + return originalClose.apply(this, args); + }); + const opError = new Error('INTERNAL_ERROR'); + opError.code = 123; + throw opError; + } + }); + + await op(filePath).catch(common.mustCall((err) => { + assert.strictEqual(err.constructor.name, 'Error'); + assert.strictEqual(err.message, 'INTERNAL_ERROR'); + assert.strictEqual(err.code, 123); + })); + } 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()); From 83f69a3765616b9c71395c351116f91377bdb530 Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Fri, 23 Apr 2021 19:31:12 +0300 Subject: [PATCH 2/3] fixup! fs: aggregate errors in fsPromises to avoid error swallowing --- ...s-promises-file-handle-aggregate-errors.js | 5 ++-- ...st-fs-promises-file-handle-close-errors.js | 20 ++++++------- .../test-fs-promises-file-handle-op-errors.js | 28 ++++++++----------- 3 files changed, 25 insertions(+), 28 deletions(-) diff --git a/test/parallel/test-fs-promises-file-handle-aggregate-errors.js b/test/parallel/test-fs-promises-file-handle-aggregate-errors.js index 35c405cc3a90e8..0479117803139f 100644 --- a/test/parallel/test-fs-promises-file-handle-aggregate-errors.js +++ b/test/parallel/test-fs-promises-file-handle-aggregate-errors.js @@ -50,12 +50,13 @@ async function checkAggregateError(op) { } }); - await op(filePath).catch(common.mustCall((err) => { - assert.strictEqual(err.constructor.name, 'AggregateError'); + 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); diff --git a/test/parallel/test-fs-promises-file-handle-close-errors.js b/test/parallel/test-fs-promises-file-handle-close-errors.js index d43f811b7c6fc0..40a33d1f003508 100644 --- a/test/parallel/test-fs-promises-file-handle-close-errors.js +++ b/test/parallel/test-fs-promises-file-handle-close-errors.js @@ -29,7 +29,7 @@ async function createFile() { return filePath; } -async function checkAggregateError(op) { +async function checkCloseError(op) { try { const filePath = await createFile(); Object.defineProperty(FileHandle.prototype, 'fd', { @@ -48,21 +48,21 @@ async function checkAggregateError(op) { } }); - await op(filePath).catch(common.mustCall((err) => { - assert.strictEqual(err.constructor.name, 'Error'); - assert.strictEqual(err.message, 'CLOSE_ERROR'); - assert.strictEqual(err.code, 456); - })); + await assert.rejects(op(filePath), { + name: 'Error', + message: 'CLOSE_ERROR', + code: 456, + }); } 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')); + await checkCloseError((filePath) => truncate(filePath)); + await checkCloseError((filePath) => readFile(filePath)); + await checkCloseError((filePath) => writeFile(filePath, '123')); if (common.isOSX) { - await checkAggregateError((filePath) => lchmod(filePath, 0o777)); + 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 index dfd378f62d5670..17f59398bc69f7 100644 --- a/test/parallel/test-fs-promises-file-handle-op-errors.js +++ b/test/parallel/test-fs-promises-file-handle-op-errors.js @@ -29,38 +29,34 @@ async function createFile() { return filePath; } -async function checkAggregateError(op) { +async function checkOperationError(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 = common.mustCall(function(...args) { - return originalClose.apply(this, args); - }); + // 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 op(filePath).catch(common.mustCall((err) => { - assert.strictEqual(err.constructor.name, 'Error'); - assert.strictEqual(err.message, 'INTERNAL_ERROR'); - assert.strictEqual(err.code, 123); - })); + await assert.rejects(op(filePath), { + name: 'Error', + message: 'INTERNAL_ERROR', + code: 123, + }); } 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')); + await checkOperationError((filePath) => truncate(filePath)); + await checkOperationError((filePath) => readFile(filePath)); + await checkOperationError((filePath) => writeFile(filePath, '123')); if (common.isOSX) { - await checkAggregateError((filePath) => lchmod(filePath, 0o777)); + await checkOperationError((filePath) => lchmod(filePath, 0o777)); } })().then(common.mustCall()); From 1dd8aae3fd1842efbb480da09a190905b789f5d7 Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Fri, 23 Apr 2021 19:34:36 +0300 Subject: [PATCH 3/3] fixup! fixup! fs: aggregate errors in fsPromises to avoid error swallowing --- test/parallel/test-fs-promises-file-handle-aggregate-errors.js | 3 +-- test/parallel/test-fs-promises-file-handle-close-errors.js | 3 +-- test/parallel/test-fs-promises-file-handle-op-errors.js | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-fs-promises-file-handle-aggregate-errors.js b/test/parallel/test-fs-promises-file-handle-aggregate-errors.js index 0479117803139f..d4c3b37d652d12 100644 --- a/test/parallel/test-fs-promises-file-handle-aggregate-errors.js +++ b/test/parallel/test-fs-promises-file-handle-aggregate-errors.js @@ -7,14 +7,13 @@ const tmpdir = require('../common/tmpdir'); // The following tests validate aggregate errors are thrown correctly // when both an operation and close throw. -const fs = require('fs'); const path = require('path'); const { readFile, writeFile, truncate, lchmod, -} = fs.promises; +} = require('fs/promises'); const { FileHandle, } = require('internal/fs/promises'); diff --git a/test/parallel/test-fs-promises-file-handle-close-errors.js b/test/parallel/test-fs-promises-file-handle-close-errors.js index 40a33d1f003508..b975b3f3e1583c 100644 --- a/test/parallel/test-fs-promises-file-handle-close-errors.js +++ b/test/parallel/test-fs-promises-file-handle-close-errors.js @@ -7,14 +7,13 @@ const tmpdir = require('../common/tmpdir'); // The following tests validate aggregate errors are thrown correctly // when both an operation and close throw. -const fs = require('fs'); const path = require('path'); const { readFile, writeFile, truncate, lchmod, -} = fs.promises; +} = require('fs/promises'); const { FileHandle, } = require('internal/fs/promises'); diff --git a/test/parallel/test-fs-promises-file-handle-op-errors.js b/test/parallel/test-fs-promises-file-handle-op-errors.js index 17f59398bc69f7..7110b5d9d40467 100644 --- a/test/parallel/test-fs-promises-file-handle-op-errors.js +++ b/test/parallel/test-fs-promises-file-handle-op-errors.js @@ -7,14 +7,13 @@ const tmpdir = require('../common/tmpdir'); // The following tests validate aggregate errors are thrown correctly // when both an operation and close throw. -const fs = require('fs'); const path = require('path'); const { readFile, writeFile, truncate, lchmod, -} = fs.promises; +} = require('fs/promises'); const { FileHandle, } = require('internal/fs/promises');