From 897f7b6c6b5ce990329f30ed1f0784db183e8e5a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 14 Mar 2018 20:11:55 +0800 Subject: fs: improve errors in watchFile and unwatchFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 Reviewed-By: Michaël Zasso Reviewed-By: Matteo Collina --- lib/fs.js | 37 ++++++++++++++++---- src/node_stat_watcher.cc | 72 +++++++++++++++++++++++++++++++------- src/node_stat_watcher.h | 2 ++ test/parallel/test-fs-watchfile.js | 7 +++- test/sequential/test-fs-watch.js | 13 +++++-- 5 files changed, 109 insertions(+), 22 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index b4c2d5cdbb1..f6722921fc5 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 00b43f6eb7c..32b416c466f 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 target) { env->SetProtoMethod(t, "start", StatWatcher::Start); env->SetProtoMethod(t, "stop", StatWatcher::Stop); + Local 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(), + static_cast(ReadOnly | DontDelete | DontEnum)); + target->Set(statWatcherString, t->GetFunction()); } @@ -73,7 +90,9 @@ StatWatcher::StatWatcher(Environment* env, Local wrap) StatWatcher::~StatWatcher() { - Stop(); + if (IsActive()) { + Stop(); + } uv_close(reinterpret_cast(watcher_), Delete); } @@ -101,32 +120,63 @@ void StatWatcher::New(const FunctionCallbackInfo& args) { new StatWatcher(env, args.This()); } +bool StatWatcher::IsActive() { + return uv_is_active(reinterpret_cast(watcher_)) != 0; +} + +void StatWatcher::IsActive(const v8::FunctionCallbackInfo& args) { + StatWatcher* wrap = Unwrap(args.This()); + CHECK(wrap != nullptr); + args.GetReturnValue().Set(wrap->IsActive()); +} +// wrap.start(filename, persistent, interval) void StatWatcher::Start(const FunctionCallbackInfo& args) { CHECK_EQ(args.Length(), 3); - StatWatcher* wrap; - ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + StatWatcher* wrap = Unwrap(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()->Value(); - if (uv_is_active(reinterpret_cast(wrap->watcher_))) - return; // Safe, uv_ref/uv_unref are idempotent. if (persistent) uv_ref(reinterpret_cast(wrap->watcher_)); else uv_unref(reinterpret_cast(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& args) { - StatWatcher* wrap; - ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + StatWatcher* wrap = Unwrap(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& args) { void StatWatcher::Stop() { - if (!uv_is_active(reinterpret_cast(watcher_))) - return; uv_fs_poll_stop(watcher_); MakeWeak(this); } diff --git a/src/node_stat_watcher.h b/src/node_stat_watcher.h index 55f4307fdbc..587203ff1ed 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& args); static void Start(const v8::FunctionCallbackInfo& args); static void Stop(const v8::FunctionCallbackInfo& args); + static void IsActive(const v8::FunctionCallbackInfo& 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 3c24ae84ac0..ba4becb2627 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 91d750acd00..3c8ae0eba7d 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 } -- cgit v1.2.3