src: make FSEventWrap/StatWatcher::Start more robust #17432

TimothyGu · 2017-12-03T02:51:22Z

Fixes: #17430

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, src

TimothyGu · 2017-12-03T02:56:48Z

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

bnoordhuis · 2017-12-03T10:36:07Z

src/fs_event_wrap.cc

-  CHECK_EQ(wrap->initialized_, false);
+  if (wrap->initialized_) {
+    args.GetReturnValue().Set(0);
+    return;


For consistency with the surrounding code, maybe write this as return args.GetReturnValue().Set(0); without braces.

bnoordhuis · 2017-12-03T10:38:02Z

test/parallel/test-fs-watch.js

@@ -64,6 +64,8 @@ for (const testCase of cases) {
      assert.strictEqual(eventType, 'change');
    assert.strictEqual(argFilename, testCase.fileName);

+    watcher.start(); // should not crash


Doesn't the linter complain about using two spaces before //?

No it doesn't, though fixed anyway.

bnoordhuis · 2017-12-03T10:49:58Z

src/node_stat_watcher.cc

@@ -111,6 +111,8 @@ void StatWatcher::Start(const FunctionCallbackInfo<Value>& args) {
  const bool persistent = args[1]->BooleanValue();
  const uint32_t interval = args[2]->Uint32Value();

+  if (uv_is_active(reinterpret_cast<uv_handle_t*>(wrap->watcher_)))
+    return;


This is wrong when Start() is called with different arguments than last time. This should work:

uv_fs_poll_stop(wrap->watcher_); uv_fs_poll_start(wrap->watcher_, Callback, *path, interval); // Safe, uv_ref/uv_unref are idempotent. if (persistent) uv_ref(reinterpret_cast<uv_handle_t*>(wrap->watcher_)); else uv_unref(reinterpret_cast<uv_handle_t*>(wrap->watcher_));

And while we're here, this code really ought to check the return value of uv_fs_poll_start() (and uv_fs_poll_stop() too, although it can't fail in the current implementation.)

Start() is not part of the external API, and no guarantees were made whether it works with different arguments or not. On the other hand, the proposed solution breaks compatibility.

Then why don't you make it throw an exception? Methods should not silently do the wrong thing, that's arguably worse than aborting.

On the other hand, the proposed solution breaks compatibility.

In what way?

The added test currently throws no exception. If I make it throw an exception it would be breaking compatibility.

What do you mean? There are no compatibility issues because it was aborting before.

It was aborting in FSEventWrap, not StatWatcher. This was simply tacked on as a similar change that improves the current behavior in a similar way.

I wouldn't use the word 'improves'. StatWatcher already worked mostly like I proposed in my first comment and that would be an acceptable change for FSEventWrap too, as would throwing an exception. Silently ignoring changes is the worst possible solution however.

Fixes: nodejs#17430

BridgeAR · 2017-12-12T22:19:55Z

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

PR-URL: nodejs#17432 Fixes: nodejs#17430 Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2017-12-13T05:45:24Z

Landed in cd71fc1

bnoordhuis · 2017-12-13T07:44:58Z

#17432 (comment) is still unfixed.

PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-01-22T08:45:23Z

should this be backported to v6.x or 8.x... my gut is no but not 100%

MylesBorins · 2018-02-27T03:04:39Z

ping re: backport

MylesBorins · 2018-05-22T15:57:50Z

ping re: backport

edit: landed cleanly on 8.x. Any reason not to land?

PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <jasnell@gmail.com>

TimothyGu · 2018-05-23T02:17:39Z

@MylesBorins No, not really.

PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <jasnell@gmail.com>

TimothyGu added the fs Issues and PRs related to the fs subsystem / file system. label Dec 3, 2017

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 Dec 3, 2017

TimothyGu mentioned this pull request Dec 3, 2017

Undocumented fs.FSWatcher::start() trips assertion in FSEventWrap::Start #17430

Closed

bnoordhuis reviewed Dec 3, 2017

View reviewed changes

src: make FSEventWrap/StatWatcher::Start more robust

b4fd89e

Fixes: nodejs#17430

TimothyGu force-pushed the fswatcher-robustify branch from 924f5f6 to b4fd89e Compare December 3, 2017 20:55

jasnell approved these changes Dec 5, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2017

addaleax pushed a commit to addaleax/node that referenced this pull request Dec 13, 2017

src: make FSEventWrap/StatWatcher::Start more robust

cd71fc1

PR-URL: nodejs#17432 Fixes: nodejs#17430 Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax closed this Dec 13, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017

TimothyGu deleted the fswatcher-robustify branch December 13, 2017 07:54

MylesBorins pushed a commit that referenced this pull request Jan 8, 2018

src: make FSEventWrap/StatWatcher::Start more robust

d879b63

PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added lts-watch-v6.x labels Jan 22, 2018

joyeecheung mentioned this pull request Jan 23, 2018

fs: migrate fs_event_wrap to internal/errors #17851

Closed

3 tasks

MylesBorins added the land-on-v8.x label May 22, 2018

MylesBorins removed the lts-watch-v8.x label May 22, 2018

MylesBorins pushed a commit that referenced this pull request May 22, 2018

src: make FSEventWrap/StatWatcher::Start more robust

4cd0ab5

PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Jun 14, 2018

src: make FSEventWrap/StatWatcher::Start more robust

7f16c8f

PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

rvagg pushed a commit that referenced this pull request Aug 16, 2018

src: make FSEventWrap/StatWatcher::Start more robust

7fa97d4

PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <jasnell@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: make FSEventWrap/StatWatcher::Start more robust #17432

src: make FSEventWrap/StatWatcher::Start more robust #17432

TimothyGu commented Dec 3, 2017

TimothyGu commented Dec 3, 2017

bnoordhuis Dec 3, 2017

bnoordhuis Dec 3, 2017

TimothyGu Dec 3, 2017

bnoordhuis Dec 3, 2017

TimothyGu Dec 3, 2017

bnoordhuis Dec 5, 2017

TimothyGu Dec 13, 2017

bnoordhuis Dec 13, 2017

TimothyGu Dec 13, 2017

bnoordhuis Dec 13, 2017

BridgeAR commented Dec 12, 2017

addaleax commented Dec 13, 2017

bnoordhuis commented Dec 13, 2017

MylesBorins commented Jan 22, 2018

MylesBorins commented Feb 27, 2018

MylesBorins commented May 22, 2018 •

edited

Loading

TimothyGu commented May 23, 2018

src: make FSEventWrap/StatWatcher::Start more robust #17432

src: make FSEventWrap/StatWatcher::Start more robust #17432

Conversation

TimothyGu commented Dec 3, 2017

Checklist

Affected core subsystem(s)

TimothyGu commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Dec 12, 2017

addaleax commented Dec 13, 2017

bnoordhuis commented Dec 13, 2017

MylesBorins commented Jan 22, 2018

MylesBorins commented Feb 27, 2018

MylesBorins commented May 22, 2018 • edited Loading

TimothyGu commented May 23, 2018

MylesBorins commented May 22, 2018 •

edited

Loading