node icon indicating copy to clipboard operation
node copied to clipboard

fs,win: fix readdir for named pipe

Open huseyinacacak-janea opened this issue 1 year ago β€’ 5 comments

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: https://github.com/nodejs/node/issues/56002 Refs: https://github.com/nodejs/node/pull/55623 Refs: https://github.com/nodejs/node/pull/56088

huseyinacacak-janea avatar Dec 02 '24 13:12 huseyinacacak-janea

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.53%. Comparing base (24a8662) to head (572569c). Report is 1402 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56110      +/-   ##
==========================================
+ Coverage   87.97%   88.53%   +0.55%     
==========================================
  Files         656      657       +1     
  Lines      188383   189858    +1475     
  Branches    35973    36447     +474     
==========================================
+ Hits       165732   168087    +2355     
+ Misses      15821    14976     -845     
+ Partials     6830     6795      -35     
Files with missing lines Coverage Ξ”
src/node_file.cc 77.41% <ΓΈ> (+0.06%) :arrow_up:

... and 135 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Dec 02 '24 14:12 codecov[bot]

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

Flarna avatar Dec 02 '24 20:12 Flarna

@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.

Flarna avatar Dec 02 '24 20:12 Flarna

CI: https://ci.nodejs.org/job/node-test-pull-request/63896/

nodejs-github-bot avatar Dec 05 '24 12:12 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/63906/

nodejs-github-bot avatar Dec 05 '24 21:12 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/63973/

nodejs-github-bot avatar Dec 10 '24 08:12 nodejs-github-bot

Landed in f184a0aca0493fba27b762d1bd8985bc06711635

nodejs-github-bot avatar Dec 10 '24 13:12 nodejs-github-bot