src: make FSEventWrap/StatWatcher::Start more robust

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

src/fs_event_wrap.cc

@@ -111,7 +111,8 @@ void FSEventWrap::Start(const FunctionCallbackInfo<Value>& args) {
 
  FSEventWrap* wrap;
  ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
- CHECK_EQ(wrap->initialized_, false);
+ if (wrap->initialized_)
+ return args.GetReturnValue().Set(0);
 
  static const char kErrMsg[] = "filename must be a string or Buffer";
  if (args.Length() < 1)

src/node_stat_watcher.cc

@@ -111,9 +111,15 @@ void StatWatcher::Start(const FunctionCallbackInfo<Value>& args) {
  const bool persistent = args[1]->BooleanValue();
  const uint32_t interval = args[2]->Uint32Value();
 
- if (!persistent)
+ if (uv_is_active(reinterpret_cast<uv_handle_t*>(wrap->watcher_)))
+ return;
+ // 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_));
  uv_fs_poll_start(wrap->watcher_, Callback, *path, interval);
+
  wrap->ClearWeak();
 }
 

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
+
  // end of test case
  watcher.close();
  }));

test/parallel/test-fs-watchfile.js

@@ -52,27 +52,30 @@ common.refreshTmpDir();
 // time, the callback should be invoked again with proper values in stat object
 let fileExists = false;
 
-fs.watchFile(enoentFile, { interval: 0 }, common.mustCall(function(curr, prev) {
- if (!fileExists) {
- // If the file does not exist, all the fields should be zero and the date
- // fields should be UNIX EPOCH time
- assert.deepStrictEqual(curr, expectedStatObject);
- assert.deepStrictEqual(prev, expectedStatObject);
- // Create the file now, so that the callback will be called back once the
- // event loop notices it.
- fs.closeSync(fs.openSync(enoentFile, 'w'));
- fileExists = true;
- } else {
- // If the ino (inode) value is greater than zero, it means that the file is
- // present in the filesystem and it has a valid inode number.
- assert(curr.ino > 0);
- // As the file just got created, previous ino value should be lesser than
- // or equal to zero (non-existent file).
- assert(prev.ino <= 0);
- // Stop watching the file
- fs.unwatchFile(enoentFile);
- }
-}, 2));
+const watcher =
+ fs.watchFile(enoentFile, { interval: 0 }, common.mustCall((curr, prev) => {
+ if (!fileExists) {
+ // If the file does not exist, all the fields should be zero and the date
+ // fields should be UNIX EPOCH time
+ assert.deepStrictEqual(curr, expectedStatObject);
+ assert.deepStrictEqual(prev, expectedStatObject);
+ // Create the file now, so that the callback will be called back once the
+ // event loop notices it.
+ fs.closeSync(fs.openSync(enoentFile, 'w'));
+ fileExists = true;
+ } else {
+ // If the ino (inode) value is greater than zero, it means that the file
+ // is present in the filesystem and it has a valid inode number.
+ assert(curr.ino > 0);
+ // As the file just got created, previous ino value should be lesser than
+ // or equal to zero (non-existent file).
+ assert(prev.ino <= 0);
+ // Stop watching the file
+ fs.unwatchFile(enoentFile);
+ }
+ }, 2));
+
+watcher.start(); // should not crash
 
 // Watch events should callback with a filename on supported systems.
 // Omitting AIX. It works but not reliably.