Skip to content

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

Closed
shackijj wants to merge 5 commits into
nodejs: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 into
nodejs:masterfrom
shackijj:fs-readdir-buffer-input-33348

Conversation

@shackijj

@shackijjshackijj commented May 14, 2020

Copy link
Copy Markdown
Contributor

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-botnodejs-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

shackijj commented May 14, 2020

Copy link
Copy Markdown
ContributorAuthor

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

@shackijj
shackijjforce-pushed the fs-readdir-buffer-input-33348 branch 4 times, most recently from f588be8 to b65f5c6CompareMay 19, 2020 04:58
@BridgeAR

Copy link
Copy Markdown
Member

@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
Copy Markdown
ContributorAuthor

@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
BridgeARforce-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72CompareMay 31, 2020 12:18
@shackijj
shackijj marked this pull request as ready for review June 13, 2020 09:12
@shackijj
shackijjforce-pushed the fs-readdir-buffer-input-33348 branch from 0133675 to 0ecdfd0CompareJune 13, 2020 13:00
@shackijj

Copy link
Copy Markdown
ContributorAuthor

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

Comment threadlib/internal/fs/utils.js Outdated
Comment threadlib/internal/fs/utils.js Outdated
@shackijj

Copy link
Copy Markdown
ContributorAuthor

Could anyone relaunch the CI pipline please?

@shackijj
shackijjforce-pushed the fs-readdir-buffer-input-33348 branch from b5d8c5c to e6d39b3CompareJune 23, 2020 18:35
@shackijj

shackijj commented Jun 23, 2020

Copy link
Copy Markdown
ContributorAuthor

Could anyone relaunch the CI pipline please?

relaunched through commit

@shackijj

Copy link
Copy Markdown
ContributorAuthor

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

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

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 :)

@addaleaxaddaleax 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
Copy Markdown
Collaborator

Comment threadlib/internal/fs/utils.js Outdated
Refs nodejs#33348
Co-authored-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot

nodejs-github-bot commented Jun 25, 2020

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

jasnell pushed a commit that referenced this pull request Jun 25, 2020
Fixes: #33348
PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

Copy link
Copy Markdown
Member

Landed in 82f13fa

@jasnelljasnell closed this Jun 25, 2020
codebytere pushed a commit that referenced this pull request Jun 27, 2020
Fixes: #33348
PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebyterecodebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
Fixes: #33348
PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jul 10, 2020
Fixes: #33348
PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jul 12, 2020
Fixes: #33348
PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebyterecodebytere mentioned this pull request Jul 13, 2020
codebytere pushed a commit that referenced this pull request Jul 14, 2020
Fixes: #33348
PR-URL: #33395
Refs: #33348
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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