Skip to content

Commit 897f7b6

Browse files
committed
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: #19345 Refs: #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>
1 parent 301f6cc commit 897f7b6

File tree

5 files changed

+109
-22
lines changed

5 files changed

+109
-22
lines changed

lib/fs.js

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1449,18 +1449,43 @@ util.inherits(StatWatcher, EventEmitter);
14491449

14501450
// FIXME(joyeecheung): this method is not documented.
14511451
// At the moment if filename is undefined, we
1452-
// 1. Throw an Error from C++ land if it's the first time .start() is called
1453-
// 2. Return silently from C++ land if .start() has already been called
1452+
// 1. Throw an Error if it's the first time .start() is called
1453+
// 2. Return silently if .start() has already been called
14541454
// on a valid filename and the wrap has been initialized
1455+
// This method is a noop if the watcher has already been started.
14551456
StatWatcher.prototype.start = function(filename, persistent, interval) {
1457+
lazyAssert()(this._handle instanceof binding.StatWatcher,
1458+
'handle must be a StatWatcher');
1459+
if (this._handle.isActive) {
1460+
return;
1461+
}
1462+
14561463
filename = getPathFromURL(filename);
1457-
nullCheck(filename, 'filename');
1458-
this._handle.start(pathModule.toNamespacedPath(filename),
1459-
persistent, interval);
1464+
validatePath(filename, 'filename');
1465+
validateUint32(interval, 'interval');
1466+
const err = this._handle.start(pathModule.toNamespacedPath(filename),
1467+
persistent, interval);
1468+
if (err) {
1469+
const error = errors.uvException({
1470+
errno: err,
1471+
syscall: 'watch',
1472+
path: filename
1473+
});
1474+
error.filename = filename;
1475+
throw error;
1476+
}
14601477
};
14611478

1462-
1479+
// FIXME(joyeecheung): this method is not documented while there is
1480+
// another documented fs.unwatchFile(). The counterpart in
1481+
// FSWatcher is .close()
1482+
// This method is a noop if the watcher has not been started.
14631483
StatWatcher.prototype.stop = function() {
1484+
lazyAssert()(this._handle instanceof binding.StatWatcher,
1485+
'handle must be a StatWatcher');
1486+
if (!this._handle.isActive) {
1487+
return;
1488+
}
14641489
this._handle.stop();
14651490
};
14661491

src/node_stat_watcher.cc

Lines changed: 60 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,19 @@
3030
namespace node {
3131

3232
using v8::Context;
33+
using v8::DontDelete;
34+
using v8::DontEnum;
3335
using v8::FunctionCallbackInfo;
3436
using v8::FunctionTemplate;
3537
using v8::HandleScope;
3638
using v8::Integer;
3739
using v8::Local;
3840
using v8::Object;
41+
using v8::PropertyAttribute;
42+
using v8::ReadOnly;
43+
using v8::Signature;
3944
using v8::String;
45+
using v8::Uint32;
4046
using v8::Value;
4147

4248

@@ -53,6 +59,17 @@ void StatWatcher::Initialize(Environment* env, Local<Object> target) {
5359
env->SetProtoMethod(t, "start", StatWatcher::Start);
5460
env->SetProtoMethod(t, "stop", StatWatcher::Stop);
5561

62+
Local<FunctionTemplate> is_active_templ =
63+
FunctionTemplate::New(env->isolate(),
64+
IsActive,
65+
env->as_external(),
66+
Signature::New(env->isolate(), t));
67+
t->PrototypeTemplate()->SetAccessorProperty(
68+
FIXED_ONE_BYTE_STRING(env->isolate(), "isActive"),
69+
is_active_templ,
70+
Local<FunctionTemplate>(),
71+
static_cast<PropertyAttribute>(ReadOnly | DontDelete | DontEnum));
72+
5673
target->Set(statWatcherString, t->GetFunction());
5774
}
5875

@@ -73,7 +90,9 @@ StatWatcher::StatWatcher(Environment* env, Local<Object> wrap)
7390

7491

7592
StatWatcher::~StatWatcher() {
76-
Stop();
93+
if (IsActive()) {
94+
Stop();
95+
}
7796
uv_close(reinterpret_cast<uv_handle_t*>(watcher_), Delete);
7897
}
7998

@@ -101,32 +120,63 @@ void StatWatcher::New(const FunctionCallbackInfo<Value>& args) {
101120
new StatWatcher(env, args.This());
102121
}
103122

123+
bool StatWatcher::IsActive() {
124+
return uv_is_active(reinterpret_cast<uv_handle_t*>(watcher_)) != 0;
125+
}
126+
127+
void StatWatcher::IsActive(const v8::FunctionCallbackInfo<v8::Value>& args) {
128+
StatWatcher* wrap = Unwrap<StatWatcher>(args.This());
129+
CHECK(wrap != nullptr);
130+
args.GetReturnValue().Set(wrap->IsActive());
131+
}
104132

133+
// wrap.start(filename, persistent, interval)
105134
void StatWatcher::Start(const FunctionCallbackInfo<Value>& args) {
106135
CHECK_EQ(args.Length(), 3);
107136

108-
StatWatcher* wrap;
109-
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
137+
StatWatcher* wrap = Unwrap<StatWatcher>(args.Holder());
138+
CHECK_NE(wrap, nullptr);
139+
if (wrap->IsActive()) {
140+
return;
141+
}
142+
143+
const int argc = args.Length();
144+
CHECK_GE(argc, 3);
145+
110146
node::Utf8Value path(args.GetIsolate(), args[0]);
111-
const bool persistent = args[1]->BooleanValue();
112-
const uint32_t interval = args[2]->Uint32Value();
147+
CHECK_NE(*path, nullptr);
148+
149+
bool persistent = true;
150+
if (args[1]->IsFalse()) {
151+
persistent = false;
152+
}
153+
154+
CHECK(args[2]->IsUint32());
155+
const uint32_t interval = args[2].As<Uint32>()->Value();
113156

114-
if (uv_is_active(reinterpret_cast<uv_handle_t*>(wrap->watcher_)))
115-
return;
116157
// Safe, uv_ref/uv_unref are idempotent.
117158
if (persistent)
118159
uv_ref(reinterpret_cast<uv_handle_t*>(wrap->watcher_));
119160
else
120161
uv_unref(reinterpret_cast<uv_handle_t*>(wrap->watcher_));
121-
uv_fs_poll_start(wrap->watcher_, Callback, *path, interval);
122162

163+
// Note that uv_fs_poll_start does not return ENOENT, we are handling
164+
// mostly memory errors here.
165+
const int err = uv_fs_poll_start(wrap->watcher_, Callback, *path, interval);
166+
if (err != 0) {
167+
args.GetReturnValue().Set(err);
168+
}
123169
wrap->ClearWeak();
124170
}
125171

126172

127173
void StatWatcher::Stop(const FunctionCallbackInfo<Value>& args) {
128-
StatWatcher* wrap;
129-
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
174+
StatWatcher* wrap = Unwrap<StatWatcher>(args.Holder());
175+
CHECK_NE(wrap, nullptr);
176+
if (!wrap->IsActive()) {
177+
return;
178+
}
179+
130180
Environment* env = wrap->env();
131181
Context::Scope context_scope(env->context());
132182
wrap->MakeCallback(env->onstop_string(), 0, nullptr);
@@ -135,8 +185,6 @@ void StatWatcher::Stop(const FunctionCallbackInfo<Value>& args) {
135185

136186

137187
void StatWatcher::Stop() {
138-
if (!uv_is_active(reinterpret_cast<uv_handle_t*>(watcher_)))
139-
return;
140188
uv_fs_poll_stop(watcher_);
141189
MakeWeak<StatWatcher>(this);
142190
}

src/node_stat_watcher.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ class StatWatcher : public AsyncWrap {
4444
static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
4545
static void Start(const v8::FunctionCallbackInfo<v8::Value>& args);
4646
static void Stop(const v8::FunctionCallbackInfo<v8::Value>& args);
47+
static void IsActive(const v8::FunctionCallbackInfo<v8::Value>& args);
4748

4849
size_t self_size() const override { return sizeof(*this); }
4950

@@ -53,6 +54,7 @@ class StatWatcher : public AsyncWrap {
5354
const uv_stat_t* prev,
5455
const uv_stat_t* curr);
5556
void Stop();
57+
bool IsActive();
5658

5759
uv_fs_poll_t* watcher_;
5860
};

test/parallel/test-fs-watchfile.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,15 @@ const watcher =
7474
assert(prev.ino <= 0);
7575
// Stop watching the file
7676
fs.unwatchFile(enoentFile);
77+
watcher.stop(); // stopping a stopped watcher should be a noop
7778
}
7879
}, 2));
7980

80-
watcher.start(); // should not crash
81+
// 'stop' should only be emitted once - stopping a stopped watcher should
82+
// not trigger a 'stop' event.
83+
watcher.on('stop', common.mustCall(function onStop() {}));
84+
85+
watcher.start(); // starting a started watcher should be a noop
8186

8287
// Watch events should callback with a filename on supported systems.
8388
// Omitting AIX. It works but not reliably.

test/sequential/test-fs-watch.js

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,20 @@ tmpdir.refresh();
122122
code: 'ERR_ASSERTION'
123123
});
124124
oldhandle.close(); // clean up
125+
}
125126

126-
assert.throws(function() {
127-
const w = fs.watchFile(__filename, { persistent: false },
127+
{
128+
let oldhandle;
129+
assert.throws(() => {
130+
const w = fs.watchFile(__filename,
131+
{ persistent: false },
128132
common.mustNotCall());
129133
oldhandle = w._handle;
130134
w._handle = { stop: w._handle.stop };
131135
w.stop();
132-
}, /^TypeError: Illegal invocation$/);
136+
}, {
137+
message: 'handle must be a StatWatcher',
138+
code: 'ERR_ASSERTION'
139+
});
133140
oldhandle.stop(); // clean up
134141
}

0 commit comments

Comments
 (0)