src: make FSEventWrap/StatWatcher::Start more robust #17432
Conversation
nodejs-github-bot
added
c++
src/fs_event_wrap.cc
Outdated
| CHECK_EQ(wrap->initialized_, false); | ||
| if (wrap->initialized_) { | ||
| args.GetReturnValue().Set(0); | ||
| return; |
There was a problem hiding this comment.
For consistency with the surrounding code, maybe write this as return args.GetReturnValue().Set(0); without braces.
test/parallel/test-fs-watch.js
Outdated
| @@ -64,6 +64,8 @@ for (const testCase of cases) { | |||
| assert.strictEqual(eventType, 'change'); | |||
| assert.strictEqual(argFilename, testCase.fileName); | |||
|
|
|||
| watcher.start(); // should not crash | |||
There was a problem hiding this comment.
Doesn't the linter complain about using two spaces before //?
There was a problem hiding this comment.
No it doesn't, though fixed anyway.
src/node_stat_watcher.cc
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The added test currently throws no exception. If I make it throw an exception it would be breaking compatibility.
There was a problem hiding this comment.
What do you mean? There are no compatibility issues because it was aborting before.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
TimothyGu
force-pushed
the
fswatcher-robustify
branch
from
924f5f6
to
b4fd89e
Compare
BridgeAR
added
the
author ready
PR-URL: nodejs#17432 Fixes: nodejs#17430 Reviewed-By: James M Snell <[email protected]>
|
Landed in cd71fc1 |
addaleax
removed
the
author ready
|
#17432 (comment) is still unfixed. |
PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <[email protected]>
|
should this be backported to v6.x or 8.x... my gut is no but not 100% |
|
ping re: backport |
|
ping re: backport edit: landed cleanly on 8.x. Any reason not to land? |
PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <[email protected]>
|
@MylesBorins No, not really. |
PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <[email protected]>
PR-URL: #17432 Fixes: #17430 Reviewed-By: James M Snell <[email protected]>
Fixes: #17430
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
fs, src