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

PR-URL: https://github.com/nodejs/node/pull/19345
Refs: https://github.com/nodejs/node/pull/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>

5 files changed, 109 insertions, 22 deletions

diff --git a/lib/fs.js b/lib/fs.js
index b4c2d5cdbb..f6722921fc 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -1449,18 +1449,43 @@ util.inherits(StatWatcher, EventEmitter);
 
 // FIXME(joyeecheung): this method is not documented.
 // At the moment if filename is undefined, we
-// 1. Throw an Error from C++ land if it's the first time .start() is called
-// 2. Return silently from C++ land if .start() has already been called
+// 1. Throw an Error if it's the first time .start() is called
+// 2. Return silently if .start() has already been called
 //    on a valid filename and the wrap has been initialized
+// This method is a noop if the watcher has already been started.
 StatWatcher.prototype.start = function(filename, persistent, interval) {
+  lazyAssert()(this._handle instanceof binding.StatWatcher,
+               'handle must be a StatWatcher');
+  if (this._handle.isActive) {
+    return;
+  }
+
   filename = getPathFromURL(filename);
-  nullCheck(filename, 'filename');
-  this._handle.start(pathModule.toNamespacedPath(filename),
-                     persistent, interval);
+  validatePath(filename, 'filename');
+  validateUint32(interval, 'interval');
+  const err = this._handle.start(pathModule.toNamespacedPath(filename),
+                                 persistent, interval);
+  if (err) {
+    const error = errors.uvException({
+      errno: err,
+      syscall: 'watch',
+      path: filename
+    });
+    error.filename = filename;
+    throw error;
+  }
 };
 
-
+// FIXME(joyeecheung): this method is not documented while there is
+// another documented fs.unwatchFile(). The counterpart in
+// FSWatcher is .close()
+// This method is a noop if the watcher has not been started.
 StatWatcher.prototype.stop = function() {
+  lazyAssert()(this._handle instanceof binding.StatWatcher,
+               'handle must be a StatWatcher');
+  if (!this._handle.isActive) {
+    return;
+  }
   this._handle.stop();
 };
 
diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc
index 00b43f6eb7..32b416c466 100644
--- a/src/node_stat_watcher.cc
+++ b/src/node_stat_watcher.cc
@@ -30,13 +30,19 @@
 namespace node {
 
 using v8::Context;
+using v8::DontDelete;
+using v8::DontEnum;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
 using v8::HandleScope;
 using v8::Integer;
 using v8::Local;
 using v8::Object;
+using v8::PropertyAttribute;
+using v8::ReadOnly;
+using v8::Signature;
 using v8::String;
+using v8::Uint32;
 using v8::Value;
 
 
@@ -53,6 +59,17 @@ void StatWatcher::Initialize(Environment* env, Local<Object> target) {
   env->SetProtoMethod(t, "start", StatWatcher::Start);
   env->SetProtoMethod(t, "stop", StatWatcher::Stop);
 
+  Local<FunctionTemplate> is_active_templ =
+      FunctionTemplate::New(env->isolate(),
+                            IsActive,
+                            env->as_external(),
+                            Signature::New(env->isolate(), t));
+  t->PrototypeTemplate()->SetAccessorProperty(
+      FIXED_ONE_BYTE_STRING(env->isolate(), "isActive"),
+      is_active_templ,
+      Local<FunctionTemplate>(),
+      static_cast<PropertyAttribute>(ReadOnly | DontDelete | DontEnum));
+
   target->Set(statWatcherString, t->GetFunction());
 }
 
@@ -73,7 +90,9 @@ StatWatcher::StatWatcher(Environment* env, Local<Object> wrap)
 
 
 StatWatcher::~StatWatcher() {
-  Stop();
+  if (IsActive()) {
+    Stop();
+  }
   uv_close(reinterpret_cast<uv_handle_t*>(watcher_), Delete);
 }
 
@@ -101,32 +120,63 @@ void StatWatcher::New(const FunctionCallbackInfo<Value>& args) {
   new StatWatcher(env, args.This());
 }
 
+bool StatWatcher::IsActive() {
+  return uv_is_active(reinterpret_cast<uv_handle_t*>(watcher_)) != 0;
+}
+
+void StatWatcher::IsActive(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  StatWatcher* wrap = Unwrap<StatWatcher>(args.This());
+  CHECK(wrap != nullptr);
+  args.GetReturnValue().Set(wrap->IsActive());
+}
 
+// wrap.start(filename, persistent, interval)
 void StatWatcher::Start(const FunctionCallbackInfo<Value>& args) {
   CHECK_EQ(args.Length(), 3);
 
-  StatWatcher* wrap;
-  ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
+  StatWatcher* wrap = Unwrap<StatWatcher>(args.Holder());
+  CHECK_NE(wrap, nullptr);
+  if (wrap->IsActive()) {
+    return;
+  }
+
+  const int argc = args.Length();
+  CHECK_GE(argc, 3);
+
   node::Utf8Value path(args.GetIsolate(), args[0]);
-  const bool persistent = args[1]->BooleanValue();
-  const uint32_t interval = args[2]->Uint32Value();
+  CHECK_NE(*path, nullptr);
+
+  bool persistent = true;
+  if (args[1]->IsFalse()) {
+    persistent = false;
+  }
+
+  CHECK(args[2]->IsUint32());
+  const uint32_t interval = args[2].As<Uint32>()->Value();
 
-  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);
 
+  // Note that uv_fs_poll_start does not return ENOENT, we are handling
+  // mostly memory errors here.
+  const int err = uv_fs_poll_start(wrap->watcher_, Callback, *path, interval);
+  if (err != 0) {
+    args.GetReturnValue().Set(err);
+  }
   wrap->ClearWeak();
 }
 
 
 void StatWatcher::Stop(const FunctionCallbackInfo<Value>& args) {
-  StatWatcher* wrap;
-  ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
+  StatWatcher* wrap = Unwrap<StatWatcher>(args.Holder());
+  CHECK_NE(wrap, nullptr);
+  if (!wrap->IsActive()) {
+    return;
+  }
+
   Environment* env = wrap->env();
   Context::Scope context_scope(env->context());
   wrap->MakeCallback(env->onstop_string(), 0, nullptr);
@@ -135,8 +185,6 @@ void StatWatcher::Stop(const FunctionCallbackInfo<Value>& args) {
 
 
 void StatWatcher::Stop() {
-  if (!uv_is_active(reinterpret_cast<uv_handle_t*>(watcher_)))
-    return;
   uv_fs_poll_stop(watcher_);
   MakeWeak<StatWatcher>(this);
 }
diff --git a/src/node_stat_watcher.h b/src/node_stat_watcher.h
index 55f4307fdb..587203ff1e 100644
--- a/src/node_stat_watcher.h
+++ b/src/node_stat_watcher.h
@@ -44,6 +44,7 @@ class StatWatcher : public AsyncWrap {
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Start(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Stop(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void IsActive(const v8::FunctionCallbackInfo<v8::Value>& args);
 
   size_t self_size() const override { return sizeof(*this); }
 
@@ -53,6 +54,7 @@ class StatWatcher : public AsyncWrap {
                        const uv_stat_t* prev,
                        const uv_stat_t* curr);
   void Stop();
+  bool IsActive();
 
   uv_fs_poll_t* watcher_;
 };
diff --git a/test/parallel/test-fs-watchfile.js b/test/parallel/test-fs-watchfile.js
index 3c24ae84ac..ba4becb262 100644
--- a/test/parallel/test-fs-watchfile.js
+++ b/test/parallel/test-fs-watchfile.js
@@ -74,10 +74,15 @@ const watcher =
       assert(prev.ino <= 0);
       // Stop watching the file
       fs.unwatchFile(enoentFile);
+      watcher.stop();  // stopping a stopped watcher should be a noop
     }
   }, 2));
 
-watcher.start();  // should not crash
+// 'stop' should only be emitted once - stopping a stopped watcher should
+// not trigger a 'stop' event.
+watcher.on('stop', common.mustCall(function onStop() {}));
+
+watcher.start();  // starting a started watcher should be a noop
 
 // Watch events should callback with a filename on supported systems.
 // Omitting AIX. It works but not reliably.
diff --git a/test/sequential/test-fs-watch.js b/test/sequential/test-fs-watch.js
index 91d750acd0..3c8ae0eba7 100644
--- a/test/sequential/test-fs-watch.js
+++ b/test/sequential/test-fs-watch.js
@@ -122,13 +122,20 @@ tmpdir.refresh();
     code: 'ERR_ASSERTION'
   });
   oldhandle.close(); // clean up
+}
 
-  assert.throws(function() {
-    const w = fs.watchFile(__filename, { persistent: false },
+{
+  let oldhandle;
+  assert.throws(() => {
+    const w = fs.watchFile(__filename,
+                           { persistent: false },
                            common.mustNotCall());
     oldhandle = w._handle;
     w._handle = { stop: w._handle.stop };
     w.stop();
-  }, /^TypeError: Illegal invocation$/);
+  }, {
+    message: 'handle must be a StatWatcher',
+    code: 'ERR_ASSERTION'
+  });
   oldhandle.stop(); // clean up
 }