fs: improve errors thrown from fs.watch()

- Add an accessor property `initialized `to FSEventWrap to check the state of the handle from the JS land - Introduce ERR_FS_WATCHER_ALREADY_STARTED so calling start() on a watcher that is already started will throw instead of doing nothing silently. - Introduce ERR_FS_WATCHER_NOT_STARTED so calling close() on a watcher that is already closed will throw instead of doing nothing silently. - Validate the filename passed to fs.watch() - Assert that the handle in the watcher are instances of FSEvent instead of relying on the illegal invocation error from the VM. - Add more assertions in FSEventWrap methods now that we check `initialized` and the filename in JS land before invoking the binding. - Use uvException instead of errornoException to create the errors with the error numbers from libuv to make them consistent with other errors in fs. TODO: - Improve fs.watchFile() the same way this patch improves fs.watch() - It seems possible to fire both rename and change event from libuv together now that we can check if the handle is closed via `initialized` in JS land. PR-URL: #19089 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
nodejs · Mar 7, 2018 · 6c25f2e · 6c25f2e
1 parent 48b5c11
commit 6c25f2e
Show file tree

Hide file tree

Showing 7 changed files with 179 additions and 55 deletions.
diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -783,6 +783,18 @@ 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
 

diff --git a/lib/fs.js b/lib/fs.js
@@ -77,13 +77,19 @@ Object.defineProperty(exports, 'constants', {
   value: constants
 });
 
+let assert_ = null;
+function lazyAssert() {
+  if (assert_ === null) {
+    assert_ = require('assert');
+  }
+  return assert_;
+}
+
 const kMinPoolSpace = 128;
 const { kMaxLength } = require('buffer');
 
 const isWindows = process.platform === 'win32';
 
-const errnoException = errors.errnoException;
-
 let truncateWarn = true;
 
 function showTruncateDeprecation() {
@@ -1312,11 +1318,17 @@ function FSWatcher() {
   this._handle.owner = this;
 
   this._handle.onchange = function(status, eventType, filename) {
+    // TODO(joyeecheung): we may check self._handle.initialized here
+    // and return if that is false. This allows us to avoid firing the event
+    // after the handle is closed, and to fire both UV_RENAME and UV_CHANGE
+    // if they are set by libuv at the same time.
     if (status < 0) {
       self._handle.close();
-      const error = !filename ?
-        errnoException(status, 'Error watching file for changes:') :
-        errnoException(status, `Error watching file ${filename} for changes:`);
+      const error = errors.uvException({
+        errno: status,
+        syscall: 'watch',
+        path: filename
+      });
       error.filename = filename;
       self.emit('error', error);
     } else {
@@ -1335,21 +1347,34 @@ FSWatcher.prototype.start = function(filename,
                                      persistent,
                                      recursive,
                                      encoding) {
+  lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent');
+  if (this._handle.initialized) {
+    throw new errors.Error('ERR_FS_WATCHER_ALREADY_STARTED');
+  }
+
   filename = getPathFromURL(filename);
-  nullCheck(filename, 'filename');
+  validatePath(filename, 'filename');
+
   var err = this._handle.start(pathModule.toNamespacedPath(filename),
                                persistent,
                                recursive,
                                encoding);
   if (err) {
-    this._handle.close();
-    const error = errnoException(err, `watch ${filename}`);
+    const error = errors.uvException({
+      errno: err,
+      syscall: 'watch',
+      path: filename
+    });
     error.filename = filename;
     throw error;
   }
 };
 
 FSWatcher.prototype.close = function() {
+  lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent');
+  if (!this._handle.initialized) {
+    throw new errors.Error('ERR_FS_WATCHER_NOT_STARTED');
+  }
   this._handle.close();
 };
 

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -658,6 +658,10 @@ 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',

diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc
@@ -31,13 +31,18 @@
 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::MaybeLocal;
 using v8::Object;
+using v8::PropertyAttribute;
+using v8::ReadOnly;
+using v8::Signature;
 using v8::String;
 using v8::Value;
 
@@ -51,7 +56,7 @@ class FSEventWrap: public HandleWrap {
   static void New(const FunctionCallbackInfo<Value>& args);
   static void Start(const FunctionCallbackInfo<Value>& args);
   static void Close(const FunctionCallbackInfo<Value>& args);
-
+  static void GetInitialized(const FunctionCallbackInfo<Value>& args);
   size_t self_size() const override { return sizeof(*this); }
 
  private:
@@ -80,6 +85,11 @@ FSEventWrap::~FSEventWrap() {
   CHECK_EQ(initialized_, false);
 }
 
+void FSEventWrap::GetInitialized(const FunctionCallbackInfo<Value>& args) {
+  FSEventWrap* wrap = Unwrap<FSEventWrap>(args.This());
+  CHECK(wrap != nullptr);
+  args.GetReturnValue().Set(wrap->initialized_);
+}
 
 void FSEventWrap::Initialize(Local<Object> target,
                              Local<Value> unused,
@@ -95,6 +105,18 @@ void FSEventWrap::Initialize(Local<Object> target,
   env->SetProtoMethod(t, "start", Start);
   env->SetProtoMethod(t, "close", Close);
 
+  Local<FunctionTemplate> get_initialized_templ =
+      FunctionTemplate::New(env->isolate(),
+                            GetInitialized,
+                            env->as_external(),
+                            Signature::New(env->isolate(), t));
+
+  t->PrototypeTemplate()->SetAccessorProperty(
+      FIXED_ONE_BYTE_STRING(env->isolate(), "initialized"),
+      get_initialized_templ,
+      Local<FunctionTemplate>(),
+      static_cast<PropertyAttribute>(ReadOnly | DontDelete | v8::DontEnum));
+
   target->Set(fsevent_string, t->GetFunction());
 }
 
@@ -105,22 +127,19 @@ void FSEventWrap::New(const FunctionCallbackInfo<Value>& args) {
   new FSEventWrap(env, args.This());
 }
 
-
+// wrap.start(filename, persistent, recursive, encoding)
 void FSEventWrap::Start(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
-  FSEventWrap* wrap;
-  ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
-  if (wrap->initialized_)
-    return args.GetReturnValue().Set(0);
+  FSEventWrap* wrap = Unwrap<FSEventWrap>(args.Holder());
+  CHECK_NE(wrap, nullptr);
+  CHECK(!wrap->initialized_);
 
-  static const char kErrMsg[] = "filename must be a string or Buffer";
-  if (args.Length() < 1)
-    return env->ThrowTypeError(kErrMsg);
+  const int argc = args.Length();
+  CHECK_GE(argc, 4);
 
   BufferValue path(env->isolate(), args[0]);
-  if (*path == nullptr)
-    return env->ThrowTypeError(kErrMsg);
+  CHECK_NE(*path, nullptr);
 
   unsigned int flags = 0;
   if (args[2]->IsTrue())
@@ -129,19 +148,21 @@ void FSEventWrap::Start(const FunctionCallbackInfo<Value>& args) {
   wrap->encoding_ = ParseEncoding(env->isolate(), args[3], kDefaultEncoding);
 
   int err = uv_fs_event_init(wrap->env()->event_loop(), &wrap->handle_);
-  if (err == 0) {
-    wrap->initialized_ = true;
+  if (err != 0) {
+    return args.GetReturnValue().Set(err);
+  }
 
-    err = uv_fs_event_start(&wrap->handle_, OnEvent, *path, flags);
+  err = uv_fs_event_start(&wrap->handle_, OnEvent, *path, flags);
+  wrap->initialized_ = true;
 
-    if (err == 0) {
-      // Check for persistent argument
-      if (!args[1]->IsTrue()) {
-        uv_unref(reinterpret_cast<uv_handle_t*>(&wrap->handle_));
-      }
-    } else {
-      FSEventWrap::Close(args);
-    }
+  if (err != 0) {
+    FSEventWrap::Close(args);
+    return args.GetReturnValue().Set(err);
+  }
+
+  // Check for persistent argument
+  if (!args[1]->IsTrue()) {
+    uv_unref(reinterpret_cast<uv_handle_t*>(&wrap->handle_));
   }
 
   args.GetReturnValue().Set(err);
@@ -209,13 +230,11 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename,
 
 
 void FSEventWrap::Close(const FunctionCallbackInfo<Value>& args) {
-  FSEventWrap* wrap;
-  ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
+  FSEventWrap* wrap = Unwrap<FSEventWrap>(args.Holder());
+  CHECK_NE(wrap, nullptr);
+  CHECK(wrap->initialized_);
 
-  if (wrap == nullptr || wrap->initialized_ == false)
-    return;
   wrap->initialized_ = false;
-
   HandleWrap::Close(args);
 }
 

diff --git a/test/parallel/test-fs-watch-enoent.js b/test/parallel/test-fs-watch-enoent.js
@@ -1,21 +1,64 @@
 'use strict';
+
+// This verifies the error thrown by fs.watch.
+
 const common = require('../common');
 const assert = require('assert');
 const fs = require('fs');
+const tmpdir = require('../common/tmpdir');
+const path = require('path');
+const nonexistentFile = path.join(tmpdir.path, 'non-existent');
+const uv = process.binding('uv');
+
+tmpdir.refresh();
+
+{
+  const validateError = (err) => {
+    assert.strictEqual(err.path, nonexistentFile);
+    assert.strictEqual(err.filename, nonexistentFile);
+    assert.strictEqual(err.syscall, 'watch');
+    if (err.code === 'ENOENT') {
+      assert.strictEqual(
+        err.message,
+        `ENOENT: no such file or directory, watch '${nonexistentFile}'`);
+      assert.strictEqual(err.errno, uv.UV_ENOENT);
+      assert.strictEqual(err.code, 'ENOENT');
+    } else {  // AIX
+      assert.strictEqual(
+        err.message,
+        `ENODEV: no such device, watch '${nonexistentFile}'`);
+      assert.strictEqual(err.errno, uv.UV_ENODEV);
+      assert.strictEqual(err.code, 'ENODEV');
+    }
+    return true;
+  };
+
+  assert.throws(
+    () => fs.watch(nonexistentFile, common.mustNotCall()),
+    validateError
+  );
+}
+
+{
+  const file = path.join(tmpdir.path, 'file-to-watch');
+  fs.writeFileSync(file, 'test');
+  const watcher = fs.watch(file, common.mustNotCall());
+
+  const validateError = (err) => {
+    assert.strictEqual(err.path, nonexistentFile);
+    assert.strictEqual(err.filename, nonexistentFile);
+    assert.strictEqual(
+      err.message,
+      `ENOENT: no such file or directory, watch '${nonexistentFile}'`);
+    assert.strictEqual(err.errno, uv.UV_ENOENT);
+    assert.strictEqual(err.code, 'ENOENT');
+    assert.strictEqual(err.syscall, 'watch');
+    fs.unlinkSync(file);
+    return true;
+  };
+
+  watcher.on('error', common.mustCall(validateError));
 
-assert.throws(function() {
-  fs.watch('non-existent-file');
-}, function(err) {
-  assert(err);
-  assert(/non-existent-file/.test(err));
-  assert.strictEqual(err.filename, 'non-existent-file');
-  return true;
-});
-
-const watcher = fs.watch(__filename);
-watcher.on('error', common.mustCall(function(err) {
-  assert(err);
-  assert(/non-existent-file/.test(err));
-  assert.strictEqual(err.filename, 'non-existent-file');
-}));
-watcher._handle.onchange(-1, 'ENOENT', 'non-existent-file');
+  // Simulate the invocation from the binding
+  watcher._handle.onchange(uv.UV_ENOENT, 'ENOENT', nonexistentFile);
+}
diff --git a/test/parallel/test-fs-watch.js b/test/parallel/test-fs-watch.js
@@ -65,10 +65,16 @@ for (const testCase of cases) {
       assert.strictEqual(eventType, 'change');
     assert.strictEqual(argFilename, testCase.fileName);
 
-    watcher.start();  // should not crash
-
+    common.expectsError(() => watcher.start(), {
+      code: 'ERR_FS_WATCHER_ALREADY_STARTED',
+      message: 'The watcher has already been started'
+    });
     // end of test case
     watcher.close();
+    common.expectsError(() => watcher.close(), {
+      code: 'ERR_FS_WATCHER_NOT_STARTED',
+      message: 'The watcher has not been started'
+    });
   }));
 
   // long content so it's actually flushed. toUpperCase so there's real change.
@@ -78,3 +84,15 @@ for (const testCase of cases) {
     fs.writeFileSync(testCase.filePath, content2);
   }, 100);
 }
+
+[false, 1, {}, [], null, undefined].forEach((i) => {
+  common.expectsError(
+    () => fs.watch(i, common.mustNotCall()),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      type: TypeError,
+      message: 'The "filename" argument must be one of ' +
+               'type string, Buffer, or URL'
+    }
+  );
+});
diff --git a/test/sequential/test-fs-watch.js b/test/sequential/test-fs-watch.js
@@ -112,12 +112,15 @@ tmpdir.refresh();
 // https://github.com/joyent/node/issues/6690
 {
   let oldhandle;
-  assert.throws(function() {
+  assert.throws(() => {
     const w = fs.watch(__filename, common.mustNotCall());
     oldhandle = w._handle;
     w._handle = { close: w._handle.close };
     w.close();
-  }, /^TypeError: Illegal invocation$/);
+  }, {
+    message: 'handle must be a FSEvent',
+    code: 'ERR_ASSERTION'
+  });
   oldhandle.close(); // clean up
 
   assert.throws(function() {