fs: improve errors thrown from fs.watch() #19089

joyeecheung · 2018-03-02T16:40:33Z

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.

This is a retake of #17851

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

fs

joyeecheung · 2018-03-02T16:42:25Z

lib/fs.js

@@ -1305,11 +1311,17 @@ function FSWatcher() {
  this._handle.owner = this;

  this._handle.onchange = function(status, eventType, filename) {
+    // TODO(joyeecheung): we may check self._handle.initialized here


cc @bnoordhuis can you check that this can be fixed this way? Thanks!

BTW, this is coming from:

node/src/fs_event_wrap.cc

Lines 161 to 171 in 9e9c516

// We're in a bind here. libuv can set both UV_RENAME and UV_CHANGE but

// the Node API only lets us pass a single event to JS land.

//

// The obvious solution is to run the callback twice, once for each event.

// However, since the second event is not allowed to fire if the handle is

// closed after the first event, and since there is no good way to detect

// closed handles, that option is out.

//

// For now, ignore the UV_CHANGE event if UV_RENAME is also set. Make the

// assumption that a rename implicitly means an attribute change. Not too

// unreasonable, right? Still, we should revisit this before v1.0.

joyeecheung · 2018-03-02T17:11:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13450/

jasnell

If @bnoordhuis is happy with it also, then LGTM

bnoordhuis

@nodejs/platform-aix The test is failing with ENODEV instead of ENOENT. Seems like a bug.

    AssertionError [ERR_ASSERTION]: 'ENODEV: no such device, watch \'/home/iojs/node-tmp/.tmp.2/non-existent\'' strictEqual 'ENOENT: no such file or directory, watch \'/home/iojs/node-tmp/.tmp.2/non-existent\''
        at Object.validateError (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/parallel/test-fs-watch-enoent.js:19:12)

bnoordhuis · 2018-03-05T06:37:22Z

lib/fs.js

-                               persistent,
-                               recursive,
+                               !!persistent,
+                               !!recursive,


Minor if correct change in behavior.

Right now, C++ land calls IsTrue(), which is essentially a strict x === true check. Using coercion here changes the observable behavior to a non-strict x == true check.

@bnoordhuis Yes, generally I think for boolean options it seems to be more intuitive to coerce them as if they are handled like if (options.something). The default cases are set in fs.watch so we should be fine in the common cases.

I personally think it is best to have a strict check. Why would we want to accept any thruthy value?

bnoordhuis · 2018-03-05T06:42:35Z

src/fs_event_wrap.cc

+      FIXED_ONE_BYTE_STRING(env->isolate(), "initialized"),
+      get_initialized_templ,
+      Local<FunctionTemplate>(),
+      static_cast<PropertyAttribute>(ReadOnly | DontDelete | v8::DontEnum));


Is there a reason to make it an accessor instead of a method?

@bnoordhuis No strong reasons other than this way you can't assign it to something else or wrap it...probably plays more nicely with inspector?

joyeecheung · 2018-03-05T20:05:06Z

@nodejs/tsc Needs one more TSC approval to land since it's semver-major..thanks!

mcollina

LGTM with a nit.

Can you also do a full citgm run?

mcollina · 2018-03-05T20:30:04Z

src/fs_event_wrap.cc

@@ -105,22 +127,19 @@ void FSEventWrap::New(const FunctionCallbackInfo<Value>& args) {
  new FSEventWrap(env, args.This());
 }

-
+// wrap.start(filename, persistent, recursive, encoding)


this line is commented out. Can you remove it?

@mcollina It's the signature of this method...probably less obvious than I thought

I think it is nice to have the signature. I am not sure what the convention for this is in C++ though. Something similar to JSDoc might be nicer?

This is more about the positions, the types are pretty obvious given the assertions below.

I do not have a strong opinion... hm... maybe change wrap.start to: Arguments: ?

joyeecheung · 2018-03-05T20:33:54Z

CITGM against master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1323/
CITGM against this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1324/

BridgeAR

LGTM. Just a few suggestions.

BridgeAR · 2018-03-06T18:37:15Z

test/parallel/test-fs-watch-enoent.js

+    assert.strictEqual(err.code, 'ENOENT');
+    assert.strictEqual(err.syscall, 'watch');
+    return true;
+  };


As a suggestion: assert.throws supports the object notation. So instead of using the function here, it could also be written as:

assert.throws( () => assert.fail(), { path: 'foo', filename: 'bar', ... } )

With functions it will be easier to adjust the validations to different platforms, e.g. some throws ENOENT while some throws ENODEV

I am not sure in what way that is easier to validate but it is only a suggestion, so shrug.

@BridgeAR It can be written like this

if (err.code === 'ENOENT') { // validate ENOENT } else { // validate ENODEV }

BridgeAR · 2018-03-06T18:38:47Z

src/fs_event_wrap.cc

@@ -105,22 +127,19 @@ void FSEventWrap::New(const FunctionCallbackInfo<Value>& args) {
  new FSEventWrap(env, args.This());
 }

-
+// wrap.start(filename, persistent, recursive, encoding)


I think it is nice to have the signature. I am not sure what the convention for this is in C++ though. Something similar to JSDoc might be nicer?

BridgeAR · 2018-03-06T18:40:33Z

lib/fs.js

-                               persistent,
-                               recursive,
+                               !!persistent,
+                               !!recursive,


I personally think it is best to have a strict check. Why would we want to accept any thruthy value?

BridgeAR · 2018-03-06T18:40:40Z

lib/fs.js

@@ -1328,21 +1340,34 @@ FSWatcher.prototype.start = function(filename,
                                     persistent,
                                     recursive,
                                     encoding) {
+  lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent');


Since very recently we have the (D)CHECK macro. I think it would be nice to replace these assert calls with those instead :-).

@BridgeAR Not unless CHECK throws instead of aborting...at least currently we only expects an error thrown in sequential/test-fs-watch.js

Well... it is a semver-major PR. But this is also just a suggestion.

BridgeAR · 2018-03-06T18:42:23Z

test/sequential/test-fs-watch.js

@@ -112,12 +112,15 @@ tmpdir.refresh();
 // https://github.com/joyent/node/issues/6690
 {
  let oldhandle;
-  assert.throws(function() {
+  common.expectsError(() => {


This change is actually not necessary. assert.throws does accept objects as well.

- 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.

joyeecheung · 2018-03-06T22:56:10Z

Reverted the non-strict check and added ENODEV checks to work around the AIX bug.

New CI: https://ci.nodejs.org/job/node-test-pull-request/13549/

BridgeAR · 2018-03-06T22:59:56Z

test/parallel/test-fs-watch-enoent.js

+
+  const validateError = (err) => {
+    assert.strictEqual(nonexistentFile, err.path);
+    assert.strictEqual(nonexistentFile, err.filename);


Nit: the arguments should be the other way around. Otherwise the error message would be weird in case of an error. The same in other places.

richardlau · 2018-03-07T05:39:39Z

test/parallel/test-fs-watch-enoent.js

+        err.message,
+        `ENODEV: no such device, watch '${nonexistentFile}'`);
+      assert.strictEqual(err.errno, uv.UV_ENODEV);
+      assert.strictEqual(err.code, 'ENODEV');


@nodejs/platform-aix is this expected behaviour?

--- a/test/parallel/test-fs-watch-enoent.js +++ b/test/parallel/test-fs-watch-enoent.js @@ -6,6 +6,7 @@ const fs = require('fs'); assert.throws(function() { fs.watch('non-existent-file'); }, function(err) { + console.log(err) assert(err); assert(/non-existent-file/.test(err)); assert.strictEqual(err.filename, 'non-existent-file');

with a console.log in the callback of the existing code, this is what I get in AIX:

{ Error: watch non-existent-file ENODEV at FSWatcher.start (fs.js:1346:19) at Object.fs.watch (fs.js:1369:11) at /tmp/gireesh/node/test/parallel/test-fs-watch-enoent.js:7:6 at getActual (assert.js:421:5) at Function.throws (assert.js:430:18) at Object.<anonymous> (/tmp/gireesh/node/test/parallel/test-fs-watch-enoent.js:6:8) at Module._compile (module.js:670:30) at Object.Module._extensions..js (module.js:681:10) at Module.load (module.js:581:32) at tryModuleLoad (module.js:521:12) errno: 'ENODEV', code: 'ENODEV', syscall: 'watch non-existent-file', filename: 'non-existent-file' }

hope this helps.

I just tested with the changes from this PR on a local AIX box, and this test looks good - the error messages are matching, thanks!

joyeecheung · 2018-03-07T13:59:56Z

Only an infra issue on ubuntu1604-32 but it was green before the update. Just in case, another Linux CI: https://ci.nodejs.org/job/node-test-commit-linux/16930/

joyeecheung · 2018-03-07T15:09:59Z

There seem to be no new CITGM errors. Linux CI is clean, landing..

- 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 <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>

joyeecheung · 2018-03-07T17:36:39Z

Landed in 6c25f2e, thanks!

- Remove ERR_FS_WATCHER_ALREADY_STARTED and ERR_FS_WATCHER_NOT_STARTED because those two situations should result in noop instead of errors for consistency with the documented behavior of fs.watchFile. This partially reverts nodejs#19089 - Update comments about this behavior. Refs: nodejs#19089

- Remove ERR_FS_WATCHER_ALREADY_STARTED and ERR_FS_WATCHER_NOT_STARTED because those two situations should result in noop instead of errors for consistency with the documented behavior of fs.watchFile. This partially reverts #19089 - Update comments about this behavior. Refs: #19089 PR-URL: #19345 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

- 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>

- 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: nodejs#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>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. labels Mar 2, 2018

joyeecheung commented Mar 2, 2018

View reviewed changes

joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 2, 2018

joyeecheung mentioned this pull request Mar 2, 2018

ENOENT error message inconsistencies #12351

Closed

jasnell approved these changes Mar 2, 2018

View reviewed changes

bnoordhuis approved these changes Mar 5, 2018

View reviewed changes

mcollina approved these changes Mar 5, 2018

View reviewed changes

BridgeAR approved these changes Mar 6, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 6, 2018

joyeecheung added 2 commits March 7, 2018 06:16

squash! strict checks, assert.throw(), ENODEV

32c7e59

joyeecheung force-pushed the fs-watch-errors branch from b5d32bb to 32c7e59 Compare March 6, 2018 22:55

BridgeAR reviewed Mar 6, 2018

View reviewed changes

richardlau reviewed Mar 7, 2018

View reviewed changes

joyeecheung closed this Mar 7, 2018

joyeecheung mentioned this pull request Mar 14, 2018

fs: improve errors from watch, watchFile and unwatchFile #19345

Closed

2 tasks

jonjaques mentioned this pull request Oct 12, 2019

FSWatcher.close() incurs a big performance cost in >=10.16 #29949

Open

	// We're in a bind here. libuv can set both UV_RENAME and UV_CHANGE but
	// the Node API only lets us pass a single event to JS land.
	//
	// The obvious solution is to run the callback twice, once for each event.
	// However, since the second event is not allowed to fire if the handle is
	// closed after the first event, and since there is no good way to detect
	// closed handles, that option is out.
	//
	// For now, ignore the UV_CHANGE event if UV_RENAME is also set. Make the
	// assumption that a rename implicitly means an attribute change. Not too
	// unreasonable, right? Still, we should revisit this before v1.0.

fs: improve errors thrown from fs.watch() #19089

fs: improve errors thrown from fs.watch() #19089

Conversation

joyeecheung commented Mar 2, 2018

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Mar 2, 2018

jasnell left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Mar 5, 2018

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Mar 5, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Mar 6, 2018

BridgeAR Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Mar 7, 2018

joyeecheung commented Mar 7, 2018

joyeecheung commented Mar 7, 2018

joyeecheung Mar 6, 2018 •

edited

Loading

BridgeAR Mar 6, 2018 •

edited

Loading