src: make FSEventWrap/StatWatcher::Start more robust

PR-URL: nodejs#17432 Fixes: nodejs#17430 Reviewed-By: James M Snell <[email protected]>
addaleax · Dec 13, 2017 · cd71fc1 · cd71fc1
1 parent 11ebaff
commit cd71fc1
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 23 deletions.
diff --git a/src/fs_event_wrap.cc b/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)

diff --git a/src/node_stat_watcher.cc b/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();
 }
 

diff --git a/test/parallel/test-fs-watch.js b/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();
   }));

diff --git a/test/parallel/test-fs-watchfile.js b/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.