fs: improve errors from watch, watchFile and unwatchFile

doc/api/errors.md

@@ -783,18 +783,6 @@ falsy value.
 An invalid symlink type was passed to the [`fs.symlink()`][] or
 [`fs.symlinkSync()`][] methods.
 
-<a id="ERR_FS_WATCHER_ALREADY_STARTED"></a>
-### ERR_FS_WATCHER_ALREADY_STARTED
-
-An attempt was made to start a watcher returned by `fs.watch()` that has
-already been started.
-
-<a id="ERR_FS_WATCHER_NOT_STARTED"></a>
-### ERR_FS_WATCHER_NOT_STARTED
-
-An attempt was made to initiate operations on a watcher returned by
-`fs.watch()` that has not yet been started.
-
 <a id="ERR_HTTP_HEADERS_SENT"></a>
 ### ERR_HTTP_HEADERS_SENT
 

lib/fs.js

@@ -36,8 +36,6 @@ const fs = exports;
 const { Buffer } = require('buffer');
 const errors = require('internal/errors');
 const {
-  ERR_FS_WATCHER_ALREADY_STARTED,
-  ERR_FS_WATCHER_NOT_STARTED,
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_CALLBACK,
   ERR_OUT_OF_RANGE
@@ -1342,25 +1340,26 @@ util.inherits(FSWatcher, 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.
 FSWatcher.prototype.start = function(filename,
                                      persistent,
                                      recursive,
                                      encoding) {
   lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent');
   if (this._handle.initialized) {
-    throw new ERR_FS_WATCHER_ALREADY_STARTED();
+    return;
   }
 
   filename = getPathFromURL(filename);
   validatePath(filename, 'filename');
 
-  var err = this._handle.start(pathModule.toNamespacedPath(filename),
-                               persistent,
-                               recursive,
-                               encoding);
+  const err = this._handle.start(pathModule.toNamespacedPath(filename),
+                                 persistent,
+                                 recursive,
+                                 encoding);
   if (err) {
     const error = errors.uvException({
       errno: err,
@@ -1372,10 +1371,11 @@ FSWatcher.prototype.start = function(filename,
   }
 };
 
+// This method is a noop if the watcher has not been started.
 FSWatcher.prototype.close = function() {
   lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent');
   if (!this._handle.initialized) {
-    throw new ERR_FS_WATCHER_NOT_STARTED();
+    return;
   }
   this._handle.close();
 };
@@ -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();
 };
 

lib/internal/errors.js

@@ -654,10 +654,6 @@ E('ERR_FALSY_VALUE_REJECTION', 'Promise was rejected with falsy value', Error);
 E('ERR_FS_INVALID_SYMLINK_TYPE',
   'Symlink type must be one of "dir", "file", or "junction". Received "%s"',
   Error); // Switch to TypeError. The current implementation does not seem right
-E('ERR_FS_WATCHER_ALREADY_STARTED',
-  'The watcher has already been started', Error);
-E('ERR_FS_WATCHER_NOT_STARTED',
-  'The watcher has not been started', Error);
 E('ERR_HTTP2_ALTSVC_INVALID_ORIGIN',
   'HTTP/2 ALTSVC frames require a valid origin', TypeError);
 E('ERR_HTTP2_ALTSVC_LENGTH',

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);
 }

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_;
 };

test/parallel/test-fs-watch.js

@@ -65,16 +65,10 @@ for (const testCase of cases) {
       assert.strictEqual(eventType, 'change');
     assert.strictEqual(argFilename, testCase.fileName);
 
-    common.expectsError(() => watcher.start(), {
-      code: 'ERR_FS_WATCHER_ALREADY_STARTED',
-      message: 'The watcher has already been started'
-    });
+    watcher.start();  // starting a started watcher should be a noop
     // end of test case
     watcher.close();
-    common.expectsError(() => watcher.close(), {
-      code: 'ERR_FS_WATCHER_NOT_STARTED',
-      message: 'The watcher has not been started'
-    });
+    watcher.close(); // closing a closed watcher should be a noop
   }));
 
   // long content so it's actually flushed. toUpperCase so there's real change.

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.

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
 }