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: https://github.com/nodejs/node/pull/19089
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

1 files changed, 46 insertions, 27 deletions

diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc
index 85a09060a1..b3fa3e8d9a 100644
--- a/src/fs_event_wrap.cc
+++ b/src/fs_event_wrap.cc
@@ -31,6 +31,8 @@
 namespace node {
 
 using v8::Context;
+using v8::DontDelete;
+using v8::DontEnum;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
 using v8::HandleScope;
@@ -38,6 +40,9 @@ 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);
 }