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

fs,win: fix readdir for named pipe #56110

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

huseyinacacak-janea
Copy link
Contributor

@huseyinacacak-janea huseyinacacak-janea commented Dec 2, 2024

This PR fixes the issue by giving the user the responsibility to add/remove trailing slashes for fs.readdir. Examples:

> require('fs').readdirSync('\\\\.\\pipe').length
Uncaught Error: ENOTDIR: not a directory, scandir '\\.\pipe'
    at Object.readdirSync (node:fs:1503:26) {
  errno: -4052,
  code: 'ENOTDIR',
  syscall: 'scandir',
  path: '\\\\.\\pipe'
}
> require('fs').readdirSync('\\\\.\\pipe\\').length
243

For additional context, most of the paths are resolved in resolve() in Node.js. This relies on normalizeString() which removes the trailing slashes. Since resolve() is widely used, any change in this function can cause many breaks as seen here.

Additionally, fs.readdir() internally relies on NtQueryDirectoryFile. This API doesn't allow us to use pipe paths without trailing slashes. On the other hand, fs.openSync() uses a different underlying API and works without trailing slashes in the physical drive paths. This difference highlights some inconsistencies across API functions, which seem to stem from the combined behaviors of libuv and the Windows API.

I'm open to suggestions.

cc: @Flarna

Fixes: #56002
Refs: #55623

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Dec 2, 2024
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.99%. Comparing base (24a8662) to head (54534dd).
Report is 22 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56110      +/-   ##
==========================================
+ Coverage   87.97%   87.99%   +0.02%     
==========================================
  Files         656      656              
  Lines      188383   188982     +599     
  Branches    35973    35994      +21     
==========================================
+ Hits       165732   166297     +565     
- Misses      15821    15846      +25     
- Partials     6830     6839       +9     
Files with missing lines Coverage Δ
src/node_file.cc 77.39% <ø> (+0.04%) ⬆️

... and 44 files with indirect coverage changes

@Flarna Flarna added the path Issues and PRs related to the path subsystem. label Dec 2, 2024
@Flarna
Copy link
Member

Flarna commented Dec 2, 2024

Verified locally that debugging using VsCode works. Seems VsCode already uses '\\\\.\\pipe\\'.

@Flarna
Copy link
Member

Flarna commented Dec 2, 2024

@nodejs/path @nodejs/fs

Please take a look. Not sure if this is the right way to address the API inconsitencies. Also not sure if the previous PR (#55623) should be marked as SemverMajor because even after this fix behavior is slightly different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node 23.2 fs lost the ability to enumerate named pipes on Windows
3 participants