Skip to content

fs: do not call Dir read callback twice - #65065

Open
shani-singh1 wants to merge 1 commit into
nodejs:mainfrom
shani-singh1:fs-dir-read-double-callback
Open

fs: do not call Dir read callback twice#65065
shani-singh1 wants to merge 1 commit into
nodejs:mainfrom
shani-singh1:fs-dir-read-double-callback

Conversation

@shani-singh1

Copy link
Copy Markdown

Dir.prototype.read(callback) invoked callback from inside a try block whose catch also calls callback:

try {
  this.#processReadResult(this.#path, result);
  const dirent = ArrayPrototypeShift(this.#bufferedEntries);
  if (this.#options.recursive && dirent.isDirectory()) {
    this.#readSyncRecursive(dirent);
  }
  callback(null, dirent);   // inside the try
} catch (error) {
  callback(error);          // fires again, with the user's own error
}

So an exception thrown by user code inside the callback is caught by the adjacent catch and handed straight back to the same callback, as if the directory read itself had failed. Two things go wrong: the callback runs twice, and a plain bug in user code is silently reported to the application as an I/O error with a nonsense message while the original stack is lost.

Reproduction

const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path');

const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'dir-'));
fs.writeFileSync(path.join(tmp, 'a.txt'), '');
fs.writeFileSync(path.join(tmp, 'b.txt'), '');

let calls = 0;
fs.opendir(tmp, (err, dir) => {
  if (err) throw err;
  dir.read((err2, dirent) => {
    calls++;
    console.log(`callback #${calls}: err=${err2 && err2.message} dirent=${dirent && dirent.name}`);
    if (calls === 1) throw new Error('bug in user callback');
  });
});
setTimeout(() => console.log(`total invocations : ${calls} (expected 1)`), 400);

On v24.11.1:

callback #1: err=null dirent=a.txt
callback #2: err=bug in user callback dirent=undefined
total invocations : 2 (expected 1)

Every other fs callback lets the exception reach 'uncaughtException' instead. For comparison, fs.readdir(tmp, () => { throw new Error('boom'); }) reports uncaughtException : boom.

doc/api/fs.md describes one call per read, made after the read completes:

After the read is completed, the callback will be called with an {fs.Dirent}, or null if there are no more directory entries to read.

The second call happens after completion, carrying an error that did not come from the read. Node also documents double invocation as an error condition in its own right (ERR_MULTIPLE_CALLBACK, "A callback was called more than once").

Change

Compute the entry inside the try and invoke the callback after it, so only failures of the read itself are reported through the callback. Both the buffered path and the asynchronous FSReqCallback path had the same shape and both are fixed.

Verification

Running the real lib/internal/fs/dir.js (before and after) against this build's fs_dir binding:

SAME  read(cb) sequence             a.txt,b.txt,c.txt,sub
SAME  read(cb) recursive            a.txt,b.txt,c.txt,d.txt,sub
SAME  async iterator                a.txt,b.txt,c.txt,sub
SAME  async iterator recursive      a.txt,b.txt,c.txt,d.txt,sub

throwing callback:
  before: calls=2  ["err=null name=a.txt","err=user cb error name=undefined"]
  after : calls=1  ["err=null name=a.txt"]

The normal read paths are unchanged; only the throwing-callback case differs.

`Dir.prototype.read(callback)` invoked `callback` from inside a `try`
block whose `catch` also calls `callback`. An exception thrown by user
code inside the callback was therefore caught and passed back into the
same callback as if the directory read itself had failed: the callback
ran twice, the second time with the user's own error, and the original
exception never reached `'uncaughtException'`.

Compute the entry inside the `try` and invoke the callback after it, so
only failures of the read itself are reported through the callback.
Both the buffered and the asynchronous read paths are affected.
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants