Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Node.js fs.readdir new param recursive caused bug #41549

Closed
3 tasks done
jiawei397 opened this issue Mar 8, 2024 · 2 comments · Fixed by #41582
Closed
3 tasks done

[Bug]: Node.js fs.readdir new param recursive caused bug #41549

jiawei397 opened this issue Mar 8, 2024 · 2 comments · Fixed by #41582
Assignees
Labels
27-x-y 28-x-y 29-x-y bug 🪲 component/node-integration status/confirmed A maintainer reproduced the bug or agreed with the feature

Comments

@jiawei397
Copy link

jiawei397 commented Mar 8, 2024

Preflight Checklist

Electron Version

27.0.4

What operating system are you using?

macOS

Operating System Version

macOS Sonoma 14.2.1

What arch are you using?

x64

Last Known Working Electron version

No response

Expected Behavior

When I use fs/promises.readdir with params withFileTypes and recursive, expect get real list of directory.

const fs = require("fs/promises");

const files = await fs.readdir(dir, { 
  withFileTypes: true,
  recursive: true
});

And the same is:

const fs = require("fs");

fs.readdir(
    dir,
    {
      withFileTypes: true,
      recursive: true
    },
    (err, files) => {
      if (err) {
        console.error('Error:', err)
      } else {
        console.log('Node:', process.version)
        console.log('files:', files.length)
      }
    }
  )

Also is readdirSync:

const fs = require('fs')

const files = fs.readdirSync(dir, {
  withFileTypes: true,
  recursive: true
})
console.log('Node:', process.version)
console.log('files:', files.length)

Actual Behavior

The electron version 27.0.4 get the list length is 2

Testcase Gist URL

No response

Additional Information

I have found the cause of the issue. It is a bug starting from Node.js v18.17.0, which was only fixed in v18.19.0. Since Electron started using Node.js v20 in version v29, the earlier versions using v18.17.1 all had issues.

Furthermore, as Node.js fs.readdir already supports the recursive parameter, it is not supported in lib/node/asar-fs-wrapper.ts. Therefore, there will be a bug when packaging if it is an internal file (when isAsar is true).

const { readdir } = fs;
fs.readdir = function (pathArgument: string, options?: { encoding?: string | null; withFileTypes?: boolean } | null, callback?: Function) {
    const pathInfo = splitPath(pathArgument);
    if (typeof options === 'function') {
      callback = options;
      options = undefined;
    }
    if (!pathInfo.isAsar) return readdir.apply(this, arguments);
    const { asarPath, filePath } = pathInfo;

    const archive = getOrCreateArchive(asarPath);
    if (!archive) {
      const error = createError(AsarError.INVALID_ARCHIVE, { asarPath });
      nextTick(callback!, [error]);
      return;
    }

    const files = archive.readdir(filePath);
    if (!files) {
      const error = createError(AsarError.NOT_FOUND, { asarPath, filePath });
      nextTick(callback!, [error]);
      return;
    }

    if (options?.withFileTypes) {
      const dirents = [];
      for (const file of files) {
        const childPath = path.join(filePath, file);
        const stats = archive.stat(childPath);
        if (!stats) {
          const error = createError(AsarError.NOT_FOUND, { asarPath, filePath: childPath });
          nextTick(callback!, [error]);
          return;
        }
        dirents.push(new fs.Dirent(file, stats.type));
      }
      nextTick(callback!, [null, dirents]);
      return;
    }

    nextTick(callback!, [null, files]);
  };

  fs.promises.readdir = util.promisify(fs.readdir);

The cause of this issue lies in a bug in fs.readdir, but in fact, the original fs.promises.readdir is not problematic. It is a separate implementation from fs.readdir, so the bug occurred in the above code due to the usage of fs.promises.readdir = util.promisify(fs.readdir);.

Moreover, the native fs.readdir may have performance issues when used recursively, as mentioned here. Please be aware of this if you intend to support it.

@Brandontam29
Copy link

Also has been broken for me for a few months already.

Electron Version

28.2.6

Operating System Version

Windows 11: WSL 2 Ubuntu 22.04

What arch are you using?

x64

@frankalbenesius
Copy link

Just ran into this, too. Thank you for filing an issue. Took me forever to realize it wasn't something I was doing wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
27-x-y 28-x-y 29-x-y bug 🪲 component/node-integration status/confirmed A maintainer reproduced the bug or agreed with the feature
Projects
No open projects
Status: 🛠️ Fixed for Next Release
Development

Successfully merging a pull request may close this issue.

4 participants