Skip to content

Comments

fs: readdir fails when libuv returns UV_DIRENT_UNKNOWN and the first arg is Buffer #33395

Closed
shackijj wants to merge 5 commits intonodejs:masterfrom
shackijj:fs-readdir-buffer-input-33348
Closed

fs: readdir fails when libuv returns UV_DIRENT_UNKNOWN and the first arg is Buffer #33395
shackijj wants to merge 5 commits intonodejs:masterfrom
shackijj:fs-readdir-buffer-input-33348

Conversation

@shackijj
Copy link
Contributor

@shackijj shackijj commented May 14, 2020 •
edited
Loading

Refs: #33348

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 14, 2020
@shackijj shackijj marked this pull request as draft May 14, 2020 07:36
@shackijj
Copy link
Contributor Author

shackijj commented May 14, 2020 •
edited
Loading

Created this PR as to check if the test fails on Ubuntu.

@shackijj shackijj force-pushed the fs-readdir-buffer-input-33348 branch 4 times, most recently from f588be8 to b65f5c6 Compare May 19, 2020 04:58
@BridgeAR
Copy link
Member

BridgeAR commented May 23, 2020

@shackijj there is a single failure on Linux:

=== release test-fs-utils-get-dirents ===
Path: parallel/test-fs-utils-get-dirents
--- stderr ---
internal/validators.js:121
    throw new ERR_INVALID_ARG_TYPE(name, 'string', value);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received an instance of Buffer
    at validateString (internal/validators.js:121:11)
    at Object.join (path.js:1039:7)
    at getDirents (internal/fs/utils.js:188:39)
    at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-fs-utils-get-dirents.js:33:5)
    at Module._compile (internal/modules/cjs/loader.js:1200:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1220:10)
    at Module.load (internal/modules/cjs/loader.js:1049:32)
    at Function.Module._load (internal/modules/cjs/loader.js:937:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47 {
  code: 'ERR_INVALID_ARG_TYPE'
}

@shackijj
Copy link
Contributor Author

shackijj commented May 23, 2020

@BridgeAR thanks. I haven't implemented a fix for the bug yet. Tests fail and it's expected for now.

I'm trying to figure out how we should handle cases when we need to join a string and a Buffer, and two Buffers. Do you have any thoughs on that?

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@shackijj shackijj mentioned this pull request Jun 12, 2020
Closed
@shackijj shackijj marked this pull request as ready for review June 13, 2020 09:12
@shackijj shackijj force-pushed the fs-readdir-buffer-input-33348 branch from 0133675 to 0ecdfd0 Compare June 13, 2020 13:00
@shackijj
Copy link
Contributor Author

shackijj commented Jun 16, 2020

@addaleax could you please take a look at this PR?

addaleax
addaleax approved these changes Jun 16, 2020
View reviewed changes
@shackijj
Copy link
Contributor Author

shackijj commented Jun 17, 2020

Could anyone relaunch the CI pipline please?

@shackijj shackijj force-pushed the fs-readdir-buffer-input-33348 branch from b5d8c5c to e6d39b3 Compare June 23, 2020 18:35
@shackijj
Copy link
Contributor Author

shackijj commented Jun 23, 2020 •
edited
Loading

Could anyone relaunch the CI pipline please?

relaunched through commit

@shackijj
Copy link
Contributor Author

shackijj commented Jun 24, 2020

@addaleax I made the changes that you suggested. Could you please review the code again? Should I do something else?

addaleax
addaleax approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks for the ping :)

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. and removed test Issues and PRs related to the tests. labels Jun 24, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 24, 2020

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

jasnell
jasnell reviewed Jun 24, 2020
View reviewed changes
@shackijj @jasnell
fs: add a space
0435053 
Refs nodejs#33348

Co-authored-by: James M Snell <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 25, 2020 •
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/32062/ CI job hung on one of the rasp pis... trying again

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 25, 2020

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

@addaleax addaleax mentioned this pull request Jun 25, 2020
Draft
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 25, 2020

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

jasnell pushed a commit that referenced this pull request Jun 25, 2020
@shackijj @jasnell
fs: fix readdir failure when libuv returns UV_DIRENT_UNKNOWN
82f13fa 
Fixes: #33348

PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

Landed in 82f13fa

@jasnell jasnell closed this Jun 25, 2020
codebytere pushed a commit that referenced this pull request Jun 27, 2020
@shackijj @codebytere
fs: fix readdir failure when libuv returns UV_DIRENT_UNKNOWN
95cbfce 
Fixes: #33348

PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@github-actions github-actions bot mentioned this pull request Jun 28, 2020
Open
15 tasks
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
This was referenced Jun 29, 2020
Open
Open
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@shackijj @codebytere
fs: fix readdir failure when libuv returns UV_DIRENT_UNKNOWN
024628a 
Fixes: #33348

PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 10, 2020
@shackijj @codebytere
fs: fix readdir failure when libuv returns UV_DIRENT_UNKNOWN
157e7d8 
Fixes: #33348

PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 12, 2020
@shackijj @codebytere
fs: fix readdir failure when libuv returns UV_DIRENT_UNKNOWN
a76fa60 
Fixes: #33348

PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Merged
codebytere pushed a commit that referenced this pull request Jul 14, 2020
@shackijj @codebytere
fs: fix readdir failure when libuv returns UV_DIRENT_UNKNOWN
47132ec 
Fixes: #33348

PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shackijj @BridgeAR @nodejs-github-bot @jasnell @addaleax