From c4d58e7dc62576209d36f6da25c533113f12b94e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BF=A0=20/=20green?= Date: Thu, 26 Oct 2023 11:35:40 +0900 Subject: [PATCH] fs: add stacktrace to fs/promises Sync functions in fs throwed an error with a stacktrace which is helpful for debugging. But functions in fs/promises throwed an error without a stacktrace. This commit adds stacktraces by calling Error.captureStacktrace and re-throwing the error. Refs: https://github.com/nodejs/node/issues/34817 PR-URL: https://github.com/nodejs/node/pull/49849 Backport-PR-URL: https://github.com/nodejs/node/pull/51127 Fixes: https://github.com/nodejs/node/issues/50160 Reviewed-By: Joyee Cheung Reviewed-By: Benjamin Gruenbaum Reviewed-By: Zeyu "Alex" Yang Reviewed-By: Moshe Atlow Reviewed-By: Jiawen Geng --- lib/internal/fs/promises.js | 327 +++++++++++++++------ test/parallel/test-fs-access.js | 6 +- test/parallel/test-fs-promises-readfile.js | 2 +- test/parallel/test-fs-promises.js | 3 +- 4 files changed, 248 insertions(+), 90 deletions(-) diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 36ff5b8f516514..a28a24de8a85ea 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -4,6 +4,7 @@ const { ArrayPrototypePush, ArrayPrototypePop, Error, + ErrorCaptureStackTrace, MathMax, MathMin, NumberIsSafeInteger, @@ -138,6 +139,15 @@ function lazyFsStreams() { const lazyRimRaf = getLazy(() => require('internal/fs/rimraf').rimrafPromises); +// By the time the C++ land creates an error for a promise rejection (likely from a +// libuv callback), there is already no JS frames on the stack. So we need to +// wait until V8 resumes execution back to JS land before we have enough information +// to re-capture the stack trace. +function handleErrorFromBinding(error) { + ErrorCaptureStackTrace(error, handleErrorFromBinding); + return PromiseReject(error); +} + class FileHandle extends EventEmitterMixin(JSTransferable) { /** * @param {InternalFSBinding.FileHandle | undefined} filehandle @@ -502,7 +512,11 @@ async function readFileHandle(filehandle, options) { checkAborted(signal); - const statFields = await binding.fstat(filehandle.fd, false, kUsePromises); + const statFields = await PromisePrototypeThen( + binding.fstat(filehandle.fd, false, kUsePromises), + undefined, + handleErrorFromBinding, + ); checkAborted(signal); @@ -533,8 +547,11 @@ async function readFileHandle(filehandle, options) { length = MathMin(size - totalRead, kReadFileBufferLength); } - const bytesRead = (await binding.read(filehandle.fd, buffer, offset, - length, -1, kUsePromises)) ?? 0; + const bytesRead = (await PromisePrototypeThen( + binding.read(filehandle.fd, buffer, offset, length, -1, kUsePromises), + undefined, + handleErrorFromBinding, + )) ?? 0; totalRead += bytesRead; if (bytesRead === 0 || @@ -582,8 +599,11 @@ async function access(path, mode = F_OK) { path = getValidatedPath(path); mode = getValidMode(mode, 'access'); - return binding.access(pathModule.toNamespacedPath(path), mode, - kUsePromises); + return await PromisePrototypeThen( + binding.access(pathModule.toNamespacedPath(path), mode, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function cp(src, dest, options) { @@ -597,10 +617,14 @@ async function copyFile(src, dest, mode) { src = getValidatedPath(src, 'src'); dest = getValidatedPath(dest, 'dest'); mode = getValidMode(mode, 'copyFile'); - return binding.copyFile(pathModule.toNamespacedPath(src), - pathModule.toNamespacedPath(dest), - mode, - kUsePromises); + return await PromisePrototypeThen( + binding.copyFile(pathModule.toNamespacedPath(src), + pathModule.toNamespacedPath(dest), + mode, + kUsePromises), + undefined, + handleErrorFromBinding, + ); } // Note that unlike fs.open() which uses numeric file descriptors, @@ -609,9 +633,12 @@ async function open(path, flags, mode) { path = getValidatedPath(path); const flagsNumber = stringToFlags(flags); mode = parseFileMode(mode, 'mode', 0o666); - return new FileHandle( - await binding.openFileHandle(pathModule.toNamespacedPath(path), - flagsNumber, mode, kUsePromises)); + return new FileHandle(await PromisePrototypeThen( + binding.openFileHandle(pathModule.toNamespacedPath(path), + flagsNumber, mode, kUsePromises), + undefined, + handleErrorFromBinding, + )); } async function read(handle, bufferOrParams, offset, length, position) { @@ -661,8 +688,11 @@ async function read(handle, bufferOrParams, offset, length, position) { if (!NumberIsSafeInteger(position)) position = -1; - const bytesRead = (await binding.read(handle.fd, buffer, offset, length, - position, kUsePromises)) || 0; + const bytesRead = (await PromisePrototypeThen( + binding.read(handle.fd, buffer, offset, length, position, kUsePromises), + undefined, + handleErrorFromBinding, + )) || 0; return { __proto__: null, bytesRead, buffer }; } @@ -673,8 +703,11 @@ async function readv(handle, buffers, position) { if (typeof position !== 'number') position = null; - const bytesRead = (await binding.readBuffers(handle.fd, buffers, position, - kUsePromises)) || 0; + const bytesRead = (await PromisePrototypeThen( + binding.readBuffers(handle.fd, buffers, position, kUsePromises), + undefined, + handleErrorFromBinding, + )) || 0; return { __proto__: null, bytesRead, buffers }; } @@ -703,15 +736,22 @@ async function write(handle, buffer, offsetOrOptions, length, position) { position = null; validateOffsetLengthWrite(offset, length, buffer.byteLength); const bytesWritten = - (await binding.writeBuffer(handle.fd, buffer, offset, - length, position, kUsePromises)) || 0; + (await PromisePrototypeThen( + binding.writeBuffer(handle.fd, buffer, offset, + length, position, kUsePromises), + undefined, + handleErrorFromBinding, + )) || 0; return { __proto__: null, bytesWritten, buffer }; } validateStringAfterArrayBufferView(buffer, 'buffer'); validateEncoding(buffer, length); - const bytesWritten = (await binding.writeString(handle.fd, buffer, offset, - length, kUsePromises)) || 0; + const bytesWritten = (await PromisePrototypeThen( + binding.writeString(handle.fd, buffer, offset, length, kUsePromises), + undefined, + handleErrorFromBinding, + )) || 0; return { __proto__: null, bytesWritten, buffer }; } @@ -725,17 +765,24 @@ async function writev(handle, buffers, position) { return { __proto__: null, bytesWritten: 0, buffers }; } - const bytesWritten = (await binding.writeBuffers(handle.fd, buffers, position, - kUsePromises)) || 0; + const bytesWritten = (await PromisePrototypeThen( + binding.writeBuffers(handle.fd, buffers, position, kUsePromises), + undefined, + handleErrorFromBinding, + )) || 0; return { __proto__: null, bytesWritten, buffers }; } async function rename(oldPath, newPath) { oldPath = getValidatedPath(oldPath, 'oldPath'); newPath = getValidatedPath(newPath, 'newPath'); - return binding.rename(pathModule.toNamespacedPath(oldPath), - pathModule.toNamespacedPath(newPath), - kUsePromises); + return await PromisePrototypeThen( + binding.rename(pathModule.toNamespacedPath(oldPath), + pathModule.toNamespacedPath(newPath), + kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function truncate(path, len = 0) { @@ -746,7 +793,11 @@ async function truncate(path, len = 0) { async function ftruncate(handle, len = 0) { validateInteger(len, 'len'); len = MathMax(0, len); - return binding.ftruncate(handle.fd, len, kUsePromises); + return await PromisePrototypeThen( + binding.ftruncate(handle.fd, len, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function rm(path, options) { @@ -767,15 +818,27 @@ async function rmdir(path, options) { } } - return binding.rmdir(path, kUsePromises); + return await PromisePrototypeThen( + binding.rmdir(path, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function fdatasync(handle) { - return binding.fdatasync(handle.fd, kUsePromises); + return await PromisePrototypeThen( + binding.fdatasync(handle.fd, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function fsync(handle) { - return binding.fsync(handle.fd, kUsePromises); + return await PromisePrototypeThen( + binding.fsync(handle.fd, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function mkdir(path, options) { @@ -789,9 +852,13 @@ async function mkdir(path, options) { path = getValidatedPath(path); validateBoolean(recursive, 'options.recursive'); - return binding.mkdir(pathModule.toNamespacedPath(path), - parseFileMode(mode, 'mode', 0o777), recursive, - kUsePromises); + return await PromisePrototypeThen( + binding.mkdir(pathModule.toNamespacedPath(path), + parseFileMode(mode, 'mode', 0o777), recursive, + kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function readdirRecursive(originalPath, options) { @@ -799,11 +866,15 @@ async function readdirRecursive(originalPath, options) { const queue = [ [ originalPath, - await binding.readdir( - pathModule.toNamespacedPath(originalPath), - options.encoding, - !!options.withFileTypes, - kUsePromises, + await PromisePrototypeThen( + binding.readdir( + pathModule.toNamespacedPath(originalPath), + options.encoding, + !!options.withFileTypes, + kUsePromises, + ), + undefined, + handleErrorFromBinding, ), ], ]; @@ -819,11 +890,15 @@ async function readdirRecursive(originalPath, options) { const direntPath = pathModule.join(path, dirent.name); ArrayPrototypePush(queue, [ direntPath, - await binding.readdir( - direntPath, - options.encoding, - true, - kUsePromises, + await PromisePrototypeThen( + binding.readdir( + direntPath, + options.encoding, + true, + kUsePromises, + ), + undefined, + handleErrorFromBinding, ), ]); } @@ -842,11 +917,15 @@ async function readdirRecursive(originalPath, options) { if (stat === 1) { ArrayPrototypePush(queue, [ direntPath, - await binding.readdir( - pathModule.toNamespacedPath(direntPath), - options.encoding, - false, - kUsePromises, + await PromisePrototypeThen( + binding.readdir( + pathModule.toNamespacedPath(direntPath), + options.encoding, + false, + kUsePromises, + ), + undefined, + handleErrorFromBinding, ), ]); } @@ -863,11 +942,15 @@ async function readdir(path, options) { if (options.recursive) { return readdirRecursive(path, options); } - const result = await binding.readdir( - pathModule.toNamespacedPath(path), - options.encoding, - !!options.withFileTypes, - kUsePromises, + const result = await PromisePrototypeThen( + binding.readdir( + pathModule.toNamespacedPath(path), + options.encoding, + !!options.withFileTypes, + kUsePromises, + ), + undefined, + handleErrorFromBinding, ); return options.withFileTypes ? getDirectoryEntriesPromise(path, result) : @@ -877,8 +960,12 @@ async function readdir(path, options) { async function readlink(path, options) { options = getOptions(options); path = getValidatedPath(path, 'oldPath'); - return binding.readlink(pathModule.toNamespacedPath(path), - options.encoding, kUsePromises); + return await PromisePrototypeThen( + binding.readlink(pathModule.toNamespacedPath(path), + options.encoding, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function symlink(target, path, type_) { @@ -903,60 +990,96 @@ async function symlink(target, path, type_) { target = getValidatedPath(target, 'target'); path = getValidatedPath(path); - return binding.symlink(preprocessSymlinkDestination(target, type, path), - pathModule.toNamespacedPath(path), - stringToSymlinkType(type), - kUsePromises); + return await PromisePrototypeThen( + binding.symlink(preprocessSymlinkDestination(target, type, path), + pathModule.toNamespacedPath(path), + stringToSymlinkType(type), + kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function fstat(handle, options = { bigint: false }) { - const result = await binding.fstat(handle.fd, options.bigint, kUsePromises); + const result = await PromisePrototypeThen( + binding.fstat(handle.fd, options.bigint, kUsePromises), + undefined, + handleErrorFromBinding, + ); return getStatsFromBinding(result); } async function lstat(path, options = { bigint: false }) { path = getValidatedPath(path); - const result = await binding.lstat(pathModule.toNamespacedPath(path), - options.bigint, kUsePromises); + const result = await PromisePrototypeThen( + binding.lstat(pathModule.toNamespacedPath(path), + options.bigint, kUsePromises), + undefined, + handleErrorFromBinding, + ); return getStatsFromBinding(result); } async function stat(path, options = { bigint: false }) { path = getValidatedPath(path); - const result = await binding.stat(pathModule.toNamespacedPath(path), - options.bigint, kUsePromises); + const result = await PromisePrototypeThen( + binding.stat(pathModule.toNamespacedPath(path), + options.bigint, kUsePromises), + undefined, + handleErrorFromBinding, + ); return getStatsFromBinding(result); } async function statfs(path, options = { bigint: false }) { path = getValidatedPath(path); - const result = await binding.statfs(pathModule.toNamespacedPath(path), - options.bigint, kUsePromises); + const result = await PromisePrototypeThen( + binding.statfs(pathModule.toNamespacedPath(path), + options.bigint, kUsePromises), + undefined, + handleErrorFromBinding, + ); return getStatFsFromBinding(result); } async function link(existingPath, newPath) { existingPath = getValidatedPath(existingPath, 'existingPath'); newPath = getValidatedPath(newPath, 'newPath'); - return binding.link(pathModule.toNamespacedPath(existingPath), - pathModule.toNamespacedPath(newPath), - kUsePromises); + return await PromisePrototypeThen( + binding.link(pathModule.toNamespacedPath(existingPath), + pathModule.toNamespacedPath(newPath), + kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function unlink(path) { path = getValidatedPath(path); - return binding.unlink(pathModule.toNamespacedPath(path), kUsePromises); + return await PromisePrototypeThen( + binding.unlink(pathModule.toNamespacedPath(path), kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function fchmod(handle, mode) { mode = parseFileMode(mode, 'mode'); - return binding.fchmod(handle.fd, mode, kUsePromises); + return await PromisePrototypeThen( + binding.fchmod(handle.fd, mode, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function chmod(path, mode) { path = getValidatedPath(path); mode = parseFileMode(mode, 'mode'); - return binding.chmod(pathModule.toNamespacedPath(path), mode, kUsePromises); + return await PromisePrototypeThen( + binding.chmod(pathModule.toNamespacedPath(path), mode, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function lchmod(path, mode) { @@ -971,50 +1094,76 @@ async function lchown(path, uid, gid) { path = getValidatedPath(path); validateInteger(uid, 'uid', -1, kMaxUserId); validateInteger(gid, 'gid', -1, kMaxUserId); - return binding.lchown(pathModule.toNamespacedPath(path), - uid, gid, kUsePromises); + return await PromisePrototypeThen( + binding.lchown(pathModule.toNamespacedPath(path), uid, gid, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function fchown(handle, uid, gid) { validateInteger(uid, 'uid', -1, kMaxUserId); validateInteger(gid, 'gid', -1, kMaxUserId); - return binding.fchown(handle.fd, uid, gid, kUsePromises); + return await PromisePrototypeThen( + binding.fchown(handle.fd, uid, gid, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function chown(path, uid, gid) { path = getValidatedPath(path); validateInteger(uid, 'uid', -1, kMaxUserId); validateInteger(gid, 'gid', -1, kMaxUserId); - return binding.chown(pathModule.toNamespacedPath(path), - uid, gid, kUsePromises); + return await PromisePrototypeThen( + binding.chown(pathModule.toNamespacedPath(path), uid, gid, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function utimes(path, atime, mtime) { path = getValidatedPath(path); - return binding.utimes(pathModule.toNamespacedPath(path), - toUnixTimestamp(atime), - toUnixTimestamp(mtime), - kUsePromises); + return await PromisePrototypeThen( + binding.utimes(pathModule.toNamespacedPath(path), + toUnixTimestamp(atime), + toUnixTimestamp(mtime), + kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function futimes(handle, atime, mtime) { atime = toUnixTimestamp(atime, 'atime'); mtime = toUnixTimestamp(mtime, 'mtime'); - return binding.futimes(handle.fd, atime, mtime, kUsePromises); + return await PromisePrototypeThen( + binding.futimes(handle.fd, atime, mtime, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function lutimes(path, atime, mtime) { path = getValidatedPath(path); - return binding.lutimes(pathModule.toNamespacedPath(path), - toUnixTimestamp(atime), - toUnixTimestamp(mtime), - kUsePromises); + return await PromisePrototypeThen( + binding.lutimes(pathModule.toNamespacedPath(path), + toUnixTimestamp(atime), + toUnixTimestamp(mtime), + kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function realpath(path, options) { options = getOptions(options); path = getValidatedPath(path); - return binding.realpath(pathModule.toNamespacedPath(path), options.encoding, kUsePromises); + return await PromisePrototypeThen( + binding.realpath(pathModule.toNamespacedPath(path), options.encoding, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function mkdtemp(prefix, options) { @@ -1030,7 +1179,11 @@ async function mkdtemp(prefix, options) { path = Buffer.concat([prefix, Buffer.from('XXXXXX')]); } - return binding.mkdtemp(path, options.encoding, kUsePromises); + return await PromisePrototypeThen( + binding.mkdtemp(path, options.encoding, kUsePromises), + undefined, + handleErrorFromBinding, + ); } async function writeFile(path, data, options) { diff --git a/test/parallel/test-fs-access.js b/test/parallel/test-fs-access.js index 5a8b85433eeefa..74e192816b0993 100644 --- a/test/parallel/test-fs-access.js +++ b/test/parallel/test-fs-access.js @@ -95,9 +95,13 @@ fs.promises.access(readOnlyFile, fs.constants.R_OK) assert.strictEqual(err.code, 'ENOENT'); assert.strictEqual(err.path, doesNotExist); }; + const expectedErrorPromise = (err) => { + expectedError(err); + assert.match(err.stack, /at async Object\.access/); + }; fs.access(doesNotExist, common.mustCall(expectedError)); fs.promises.access(doesNotExist) - .then(common.mustNotCall(), common.mustCall(expectedError)) + .then(common.mustNotCall(), common.mustCall(expectedErrorPromise)) .catch(throwNextTick); } diff --git a/test/parallel/test-fs-promises-readfile.js b/test/parallel/test-fs-promises-readfile.js index 9bce123537add6..ccf7aa16b12e12 100644 --- a/test/parallel/test-fs-promises-readfile.js +++ b/test/parallel/test-fs-promises-readfile.js @@ -72,7 +72,7 @@ async function validateWrongSignalParam() { async function validateZeroByteLiar() { const originalFStat = fsBinding.fstat; fsBinding.fstat = common.mustCall( - () => (/* stat fields */ [0, 1, 2, 3, 4, 5, 6, 7, 0 /* size */]) + async () => (/* stat fields */ [0, 1, 2, 3, 4, 5, 6, 7, 0 /* size */]) ); const readBuffer = await readFile(fn); assert.strictEqual(readBuffer.toString(), largeBuffer.toString()); diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index 383884fa62ed42..2dc0ae8cb44f6d 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -57,7 +57,8 @@ assert.strictEqual( { code: 'ENOENT', name: 'Error', - message: /^ENOENT: no such file or directory, access/ + message: /^ENOENT: no such file or directory, access/, + stack: /at async Function\.rejects/ } ).then(common.mustCall());