diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js index f227b51af03669..969ada8331095f 100644 --- a/lib/internal/fs/dir.js +++ b/lib/internal/fs/dir.js @@ -115,22 +115,22 @@ class Dir { return; } + let dirent; if (this.#processHandlerQueue()) { try { - const dirent = ArrayPrototypeShift(this.#bufferedEntries); + dirent = ArrayPrototypeShift(this.#bufferedEntries); if (this.#options.recursive && dirent.isDirectory()) { this.readSyncRecursive(dirent); } - - if (maybeSync) - process.nextTick(callback, null, dirent); - else - callback(null, dirent); - return; } catch (error) { return callback(error); } + if (maybeSync) + process.nextTick(callback, null, dirent); + else + callback(null, dirent); + return; } const req = new FSReqCallback(); @@ -145,16 +145,17 @@ class Dir { return callback(err, result); } + let dirent; try { this.processReadResult(this.#path, result); - const dirent = ArrayPrototypeShift(this.#bufferedEntries); + dirent = ArrayPrototypeShift(this.#bufferedEntries); if (this.#options.recursive && dirent.isDirectory()) { this.readSyncRecursive(dirent); } - callback(null, dirent); } catch (error) { - callback(error); + return callback(error); } + return callback(null, dirent); }; this.#operationQueue = []; diff --git a/test/parallel/test-fs-opendir.js b/test/parallel/test-fs-opendir.js index fda730097918be..c8f3515222455c 100644 --- a/test/parallel/test-fs-opendir.js +++ b/test/parallel/test-fs-opendir.js @@ -4,6 +4,7 @@ const common = require('../common'); const assert = require('assert'); const fs = require('fs'); const path = require('path'); +const test = require('node:test'); const tmpdir = require('../common/tmpdir'); @@ -287,3 +288,54 @@ doConcurrentAsyncMixedOps().then(common.mustCall()); dir.closeSync(); assert.rejects(dir.close(), dirclosedError).then(common.mustCall()); } + +test('fs.opendir should not double callback if user callback throws', async (t) => { + let readCallbackInvokedCount = 0; + let userErrorThrown = false; + + const dir = await fs.promises.opendir('.'); + + t.after(() => dir.close().catch((err) => console.error('Error closing dir in t.after:', err))); + + await new Promise((resolve, reject) => { + const operationTimeout = setTimeout(() => { + if (userErrorThrown && readCallbackInvokedCount === 1) { + resolve(); // User error handled, no double callback + } else if (readCallbackInvokedCount === 0) { + reject(new Error('dir.read callback was never invoked.')); + } else { + reject(new Error('Test timeout reached in an unexpected state.')); + } + }, 1000); + + dir.read((err, dirent) => { + readCallbackInvokedCount++; + + if (err) { + // This is an error from dir.read() itself, not the user error we're testing. + clearTimeout(operationTimeout); + return reject(new Error(`dir.read reported an error: ${err.message}`)); + } + + if (readCallbackInvokedCount === 1) { + userErrorThrown = true; + // Simulate the user's code throwing an error. + // A robust fs.Dir should catch this internally and not call this callback again. + throw new Error('Simulated user error in dir.read callback'); + } + + if (readCallbackInvokedCount > 1) { + clearTimeout(operationTimeout); + return reject(new Error('dir.read callback was invoked multiple times.')); + } + + // If dirent is null and it's the first call, and no user error was meant to be thrown yet, + // it implies an empty directory or an issue with the test setup if we expected more entries. + // For this specific test, we expect the user error to be thrown on the first entry. + if (dirent === null && readCallbackInvokedCount === 1 && !userErrorThrown) { + clearTimeout(operationTimeout); + return reject(new Error('Directory reading finished before user error could be simulated.')); + } + }); + }); +});