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.

Sign up for GitHub

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

Jump to bottom

fs: streamline EISDIR error on different platforms #33831

Closed
wants to merge 3 commits into from
Closed

fs: streamline EISDIR error on different platforms #33831

wants to merge 3 commits into from

Conversation

lundibundi
Copy link
Member

@lundibundi lundibundi commented Jun 10, 2020 •
edited

Previously upon calling file-only fs functions with a directory (or
presumably a directory of path ending with a forward slash '/') it would
result in EISDIR error on Linux but Windows would completely ignore trailing '/'
and perform operation on a file excluding it.

This introduces additional checks in fs code to make sure the trailing
slash is handled the same way across systems.

Fixes: #17801

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added (anything to add in the fs docs?)
  • commit message follows commit guidelines

This currently adds checks in

  • readFile/readFileSync
  • open/openSync
  • truncate/truncateSync
  • writeFile/ writeFileSync
  • readFile/ readFileSync
  • createReadStream/createReadStreamSync
  • createWriteStream/createWriteStreamSync

anything else I've missed?

/cc @nodejs/fs

@lundibundi lundibundi added the fs Issues and PRs related to the fs subsystem / file system. label Jun 10, 2020
@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jun 10, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 11, 2020
@addaleax
Copy link
Member

I know it’s far from ideal that we the introduction of our internal error system also meant mixing POSIX/libuv errors with our own error codes, but since we already have EISDIR, I think it makes sense to keep using that…?

@lundibundi
Copy link
Member Author

@addaleax done.

ping @nodejs/fs, this is ready for review.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Jun 18, 2020
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Aug 12, 2020
Open
Open
@lundibundi lundibundi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@lundibundi
Copy link
Member Author

lundibundi commented Aug 12, 2020 •
edited

/cc @addaleax @jasnell @ronag the implementation changed slightly to handle backward slash on Windows.

Also, anyone knows what can I do about test timeout on ARM? Should I separate these into a few tests? (On my machine the whole test takes less than a second though)

Edit: now also handles fs.promises (forgot about them the previous time)

This was referenced Aug 13, 2020
Open
Open
Open
Open
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Aug 17, 2020
Open
Open
@lundibundi
fs: streamline EISDIR error on different platforms
0a9e6d8 
Previously upon calling file-only fs functions with a directory (or
presumably a directory of path ending with a forward slash '/') it would
result in EISDIR error on Linux but Windows would completely ignore trailing '/'
and perform operation on a file excluding it.

This introduces additional checks in fs code to make sure the trailing
slash is handled the same way across systems.

Fixes: nodejs#17801
@lundibundi
Copy link
Member Author

@jasnell @addaleax @ronag @nodejs/fs this could use another review since implementation has changed slightly.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@lundibundi lundibundi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@lundibundi
Copy link
Member Author

ping @addaleax @jasnell @ronag @nodejs/fs could this get another review since the implementation changed slightly and also I'd especially want to hear your opinion on this (is there a better way?)
https://github.com/nodejs/node/pull/33831/files#diff-d608fd0cbcac8df44557d8e49e206c7dR38-R41

@lundibundi lundibundi added the review wanted PRs that need reviews. label Sep 13, 2020
@aduh95
Copy link
Contributor

aduh95 commented Jan 9, 2021

This needs a rebase.

@BridgeAR
Copy link
Member

Ping @lundibundi

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Sep 15, 2021
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@github-actions
Copy link
Contributor

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

@github-actions github-actions bot closed this Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell Awaiting requested review from jasnell

@addaleax addaleax Awaiting requested review from addaleax

@ronag ronag Awaiting requested review from ronag

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. review wanted PRs that need reviews. semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent behavior with fs module accross platforms
7 participants
@lundibundi @nodejs-github-bot @addaleax @aduh95 @BridgeAR @jasnell @ronag