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: migrate more fs.*Sync errors into JS land #18348

Closed
wants to merge 10 commits into from
Closed

fs: migrate more fs.*Sync errors into JS land #18348

wants to merge 10 commits into from

Conversation

joyeecheung
Copy link
Member

cc @jasnell

Refs: #18106

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

fs

@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. labels Jan 24, 2018
@joyeecheung
Copy link
Member Author

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

targos
targos approved these changes Jan 24, 2018
View reviewed changes
src/node_file.cc Outdated
&error);
link_path,
encoding,
&error);
Copy link
Member

Choose a reason for hiding this comment

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

alignment seems off?

Copy link
Member Author

Choose a reason for hiding this comment

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

@targos Yep, I'll fix that later, thanks for catching. Wondering why the cpp linter didn't catch that..

@joyeecheung joyeecheung added the wip Issues and PRs that are still a work in progress. label Jan 25, 2018
@joyeecheung
Copy link
Member Author

There is a bug in fs.symlink, fixing

@joyeecheung
Copy link
Member Author

Rebased and updated. CI: https://ci.nodejs.org/job/node-test-pull-request/12771/

@joyeecheung joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 27, 2018
@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 29, 2018 •
edited
Loading

There was a failure in test-fs-read-stream-err although from what I can tell that test does not run any code patch touched here (since they are all synchronous), and the way that test monkey-patches fs.read does not seem to be very reliable considering the pool and the GC..

Just in case, another CI: https://ci.nodejs.org/job/node-test-pull-request/12790/
Stress test against master: https://ci.nodejs.org/job/node-stress-single-test/1758/
Stress test against this PR: https://ci.nodejs.org/job/node-stress-single-test/1759/

@joyeecheung joyeecheung removed the wip Issues and PRs that are still a work in progress. label Jan 29, 2018
@joyeecheung
Copy link
Member Author

CI failures are unrelated. I can reproduce test-fs-read-stream-err in master: https://ci.nodejs.org/job/node-stress-single-test/1758/nodes=debian8-x86

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 29, 2018
@BridgeAR BridgeAR mentioned this pull request Jan 29, 2018
Open
@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 29, 2018 •
edited
Loading

Rebased and squashed fixes. @targos @jasnell can you take a look again and see if this still LGTY? Thanks!

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

@joyeecheung joyeecheung mentioned this pull request Jan 29, 2018
Closed
targos
targos approved these changes Jan 29, 2018
View reviewed changes
lib/fs.js Outdated
@@ -1163,7 +1163,13 @@ fs.readlinkSync = function(path, options) {
handleError((path = getPathFromURL(path)));
nullCheck(path);
validatePath(path, 'oldPath');
return binding.readlink(pathModule.toNamespacedPath(path), options.encoding);
const ctx = { path: path };
Copy link
Member

Choose a reason for hiding this comment

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

nit: use shorthand syntax?

@joyeecheung joyeecheung mentioned this pull request Feb 3, 2018
Closed
2 tasks
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018
joyeecheung added a commit that referenced this pull request Feb 8, 2018
@joyeecheung
fs: fix misplaced errors in fs.symlinkSync
b1c6ecb 
The ctx.error is supposed to be handled in fs.readlinkSync,
but was handled in fs.symlinkSync by mistake.

Also fix the error number check in readlink to be consistent
with SYNC_CALL.

PR-URL: #18548
Refs: #18348
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@rudidude86 rudidude86 mentioned this pull request Apr 25, 2018
Closed
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
fs: throw errors from fs.symlinkSync in JS
8ab1549 
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
fs: throw errors from fs.linkSync in JS
829f8b7 
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
fs: throw errors from fs.readlinkSync in JS
c8deefe 
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
fs: throw errors from fs.renameSync in JS
05d6940 
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
fs: throw errors from fs.ftruncateSync in JS
49f6646 
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
fs: throw errors from fs.fdatasyncSync in JS
d5196d3 
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
fs: throw errors from fs.fsyncSync in JS
3195da9 
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
fs: throw errors from fs.unlinkSync in JS
77cbe27 
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
fs: fix misplaced errors in fs.symlinkSync
f6493d2 
The ctx.error is supposed to be handled in fs.readlinkSync,
but was handled in fs.symlinkSync by mistake.

Also fix the error number check in readlink to be consistent
with SYNC_CALL.

PR-URL: nodejs#18548
Refs: nodejs#18348
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@joyeecheung @jasnell @targos @addaleax @nodejs-github-bot