Skip to content

fs: improve errors from watch, watchFile and unwatchFile - #19345

Closed
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:fs-watchfile
Closed

fs: improve errors from watch, watchFile and unwatchFile#19345
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:fs-watchfile

Conversation

@joyeecheung

@joyeecheungjoyeecheung commented Mar 14, 2018

Copy link
Copy Markdown
Member

fs: remove watcher state errors for fs.watch

  • Remove ERR_FS_WATCHER_ALREADY_STARTED and
    ERR_FS_WATCHER_NOT_STARTED because those two situations should
    result in noop instead of errors for consistency with the
    documented behavior of fs.watchFile.
    This partially reverts fs: improve errors thrown from fs.watch() #19089
  • Update comments about this behavior.

Refs: #19089

fs: improve errors in watchFile and unwatchFile

  • Check if the watcher is active in JS land before
    invoking the binding, act as a noop if the state of
    the watcher does not match the expectation. This
    avoids firing 'stop' when the watcher is already
    stopped.
  • Update comments, validate more arguments and
    the type of the handle.
  • Handle the errors from uv_fs_poll_start
  • Create an IsActive helper method on StatWatcher
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@joyeecheung
fs: remove watcher state errors for fs.watch
2463216
- Remove ERR_FS_WATCHER_ALREADY_STARTED and
ERR_FS_WATCHER_NOT_STARTED because those two situations should
result in noop instead of errors for consistency with the
documented behavior of fs.watchFile.
This partially reverts nodejs#19089
- Update comments about this behavior.
Refs: nodejs#19089
@joyeecheung
fs: improve errors in watchFile and unwatchFile
9a41128
- Check if the watcher is active in JS land before
invoking the binding, act as a noop if the state of
the watcher does not match the expectation. This
avoids firing 'stop' when the watcher is already
stopped.
- Update comments, validate more arguments and
the type of the handle.
- Handle the errors from uv_fs_poll_start
- Create an `IsActive` helper method on StatWatcher
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 14, 2018
@joyeecheungjoyeecheung added 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. labels Mar 14, 2018
@joyeecheung

joyeecheung commented Mar 14, 2018

Copy link
Copy Markdown
MemberAuthor

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

cc @nodejs/tsc this is semver-major because of the additional throw. Please review, thanks!

@joyeecheungjoyeecheung added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Mar 14, 2018
@joyeecheung

joyeecheung commented Mar 18, 2018

Copy link
Copy Markdown
MemberAuthor

@nodejs/tsc this needs one more approval to land..thanks!

mcollina
mcollina approved these changes Mar 18, 2018
View reviewed changes

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

LGTM

@joyeecheung

joyeecheung commented Mar 18, 2018

Copy link
Copy Markdown
MemberAuthor

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

@joyeecheung

joyeecheung commented Mar 18, 2018

Copy link
Copy Markdown
MemberAuthor

Landed in 897cec4...897f7b6, thanks!

joyeecheung added a commit that referenced this pull request Mar 18, 2018
@joyeecheung
fs: remove watcher state errors for fs.watch
301f6cc
- Remove ERR_FS_WATCHER_ALREADY_STARTED and
ERR_FS_WATCHER_NOT_STARTED because those two situations should
result in noop instead of errors for consistency with the
documented behavior of fs.watchFile.
This partially reverts #19089
- Update comments about this behavior.
Refs: #19089
PR-URL: #19345
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
joyeecheung added a commit that referenced this pull request Mar 18, 2018
@joyeecheung
fs: improve errors in watchFile and unwatchFile
897f7b6
- Check if the watcher is active in JS land before
invoking the binding, act as a noop if the state of
the watcher does not match the expectation. This
avoids firing 'stop' when the watcher is already
stopped.
- Update comments, validate more arguments and
the type of the handle.
- Handle the errors from uv_fs_poll_start
- Create an `IsActive` helper method on StatWatcher
PR-URL: #19345
Refs: #19089
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jonjaquesjonjaques mentioned this pull request Oct 12, 2019
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@jasnelljasnelljasnell approved these changes
@targostargostargos approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.errorsIssues and PRs related to JavaScript errors originated in Node.js core.fsIssues and PRs related to the fs subsystem / file system.lib / srcIssues and PRs related to general changes in the lib or src directory.semver-majorPRs 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@mcollina@jasnell@targos@nodejs-github-bot