fs.watchFile fixes #2093

brendanashworth · 2015-07-02T07:58:23Z

There were some issues with fs.watchFile:

no tests
bad argument check on non-function but truthy listener
no doc comment about change in functionality since v0.10

This fixes them. Supersedes #2028, fixes #1745.
/cc @thefourtheye @bnoordhuis @evanlucas

thefourtheye · 2015-07-02T08:07:21Z

test/parallel/test-fs-watchfile.js

+// Test ENOENT. Should fire once.
+var enoentFile = path.join(fixtures, 'empty', 'non-existent-file');
+fs.watchFile(enoentFile, function(curr, prev) {
+    callbackFired++;


Wouldn't it be better if we had the condition to check if the callback is really for ENOENT, as suggested here #2028 (comment)?

I'd think it'd be better too, but I'm not sure how to ensure it is ENOENT. Is checking curr.ino > 0 the only way/right way?

edit: cause I don't know what ino stands for :/

@brendanashworth I think it is inode number. cc @bnoordhuis

Yes, it's the inode number. curr.ino > 0 is just an example, though; on error, all the fields will be zero. It would be good to document that behavior in the fs.watchFile() documentation.

@bnoordhuis I was thinking, once this lands, I ll include the checks for non-existent file in fs.Stats section and mention that in fs.watchFile section. That should be fine, right?

I don't think fs.Stats is the right place for that, it's specific to fs.watchFile().

thefourtheye · 2015-07-02T08:52:30Z

Had only minor comments. It can land as-is, I guess.

kanongil · 2015-07-02T09:25:58Z

Why would you change the behavior on non-existant files? It doesn't make much sense if the callback is triggered immediately, without any actual changes, as I reported in #1745.

bnoordhuis · 2015-07-02T10:31:31Z

@kanongil See this comment. Executive summary: no callback means you can't distinguish between no file and no events.

kanongil · 2015-07-02T11:39:21Z

@bnoordhuis Ah, I see. I guess this makes sense.

When the listener was truthy but NOT a function, fs.watchFile would throw an error through the EventEmitter. This caused a problem because it would only be thrown after the listener was started, which left the listener on. There should be no backwards compatability issues because the error was always thrown, just in a different manner. Also adds tests for this and other basic functionality.

brendanashworth · 2015-07-02T19:08:50Z

Pushed up fixed commits, PTAL @thefourtheye @bnoordhuis.

thefourtheye · 2015-07-02T19:18:58Z

test/parallel/test-fs-watchfile.js

+fs.watchFile(enoentFile, function(curr, prev) {
+  callbackFired++;
+
+  fs.unwatchFile(enoentFile);


I am not sure if unwatching here is a good idea. If some change triggers the callback more than once, even in the ENOENT case, then this testcase will miss that, as it is unwatching immediately in the first invocation itself. @bnoordhuis What do you think?

What's the alternative?

I am not sure if it will work fine but how about unwatching at the process's exit event?

It won't exit without unwatching, as the watcher will keep the process open.

Oh, then I am out of ideas. We can leave it as it is I guess. Thanks for clarifying @brendanashworth :-)

@trevnorris Can you please explain the making it not persistent part? Actually the original PR had a setTimeout of 2ms and unwatch was called from there. You meant something like that?

Reference documentation: https://iojs.org/api/fs.html#fs_fs_watchfile_filename_options_listener

persistent indicates whether the process should continue to run as long as files are being watched

I'm assuming that a file that doesn't exist can't be watched. Thus allowing the process to shutdown.

I don't think that would work. { persistent: false } unrefs the handle immediately. io.js would quit as soon as the script finished, before the callback has a chance to run.

FWIW, the test looks fine to me.

That's what I was wondering.

When I had a timer in, the test was admittedly very flaky (10-20% failure rate).

bnoordhuis · 2015-07-02T19:26:52Z

LGTM FWIW.

brendanashworth · 2015-07-03T19:18:11Z

@thefourtheye LGTY?

thefourtheye · 2015-07-03T19:30:01Z

@brendanashworth I was okay with the initial version itself ;-) LGTM.

brendanashworth · 2015-07-03T19:40:11Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/92/

bnoordhuis · 2015-07-04T09:01:55Z

LGTM

thefourtheye · 2015-07-06T14:23:13Z

test/parallel/test-fs-watchfile.js

+
+// Test ENOENT. Should fire once.
+const enoentFile = path.join(fixtures, 'empty', 'non-existent-file');
+fs.watchFile(enoentFile, function(curr, prev) {


Can we simply use common.mustCall here and get rid of the counter?

I had the counter in so that if the callback were fired more than once, it would error. I'm not sure if that is still possible unless it is called synchronously twice - but just in case, I'd like to err on the side of keeping the counter.

common.mustCall() checks the call count at exit. You don't strictly need a counter here.

I will change this.

brendanashworth · 2015-07-06T18:53:56Z

I plan on merging tonight.

edit: nope nevermind

When fs.watchFile encounters an ENOENT error, it invokes the given callback with some error data. This caused an issue as it was different behaviour than Node v0.10. Instead of changing this behaviour, document it and add a test. Ref: nodejs#1745 Ref: nodejs#2028

brendanashworth · 2015-07-10T08:56:01Z

Force pushed with common.mustCall, ptal @bnoordhuis @thefourtheye .

thefourtheye · 2015-07-10T09:44:35Z

test/parallel/test-fs-watchfile.js

+// Test ENOENT. Should fire once.
+const enoentFile = path.join(fixtures, 'empty', 'non-existent-file');
+fs.watchFile(enoentFile, common.mustCall(function(curr, prev) {
+  fs.unwatchFile(enoentFile);


Oh, we are not doing inode check to confirm to see if the file really doesn't exist. Meh. Its okay I guess. LGTM 👍

bnoordhuis · 2015-07-10T10:37:25Z

One more LGTM, with a documentation suggestion. Can you run the CI before landing?

brendanashworth · 2015-07-10T21:15:41Z

CI is happy: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/141/.

@thefourtheye would you like to submit a follow up PR with the doc addition? I don't know enough to make a good writeup.

thefourtheye · 2015-07-10T21:18:04Z

@brendanashworth I think its high time you landed this ;-) We can discuss on this thread about the documentation even after that :-)

When the listener was truthy but NOT a function, fs.watchFile would throw an error through the EventEmitter. This caused a problem because it would only be thrown after the listener was started, which left the listener on. There should be no backwards compatability issues because the error was always thrown, just in a different manner. Also adds tests for this and other basic functionality. PR-URL: #2093 Reviewed-By: Ben Noordhuis <[email protected]>

When fs.watchFile encounters an ENOENT error, it invokes the given callback with some error data. This caused an issue as it was different behaviour than Node v0.10. Instead of changing this behaviour, document it and add a test. Ref: #1745 Ref: #2028 Reviewed-By: Ben Noordhuis <[email protected]> PR-URL: #2093

brendanashworth · 2015-07-10T21:26:42Z

Thank you reviewers, sorry for the lag. Merged in 12bc397...23efb05.

thefourtheye · 2015-07-10T21:27:56Z

@brendanashworth Awesome :-) Thanks :-)

ronkorving · 2015-07-23T03:08:09Z

lib/fs.js

  }

-  if (!listener) {
+  if (typeof listener !== 'function') {
    throw new Error('watchFile requires a listener function');


A bit late, since already merged, but shouldn't this be a TypeError?

Never too late :) you're right though, if you wanted to submit a PR I'd review it.

As per the discussion in nodejs#2093 (comment), this patch improves the Note section in watchFile to clarify about the non-existent files. This patch also includes a test which checks if a file not present, the callback is invoked at least once and if the file is created after the callback is invoked, it will be invoked again with new stat objects. PR-URL: nodejs#2169

As per the discussion in #2093 (comment), this patch documents the behavior of calling fs.watchFile() with a path that does not yet exist. This patch also includes a test which checks if a file not present, the callback is invoked at least once and if the file is created after the callback is invoked, it will be invoked again with new stat objects. PR-URL: #2169 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brendan Ashworth <[email protected]>

brendanashworth added fs Issues and PRs related to the fs subsystem / file system. doc Issues and PRs related to the documentations. labels Jul 2, 2015

brendanashworth mentioned this pull request Jul 2, 2015

fs: fix StatWatcher to handle error codes #2028

Closed

thefourtheye reviewed Jul 2, 2015
View reviewed changes

brendanashworth force-pushed the new/fs-watchfile-tests branch from 4d5655d to d9c795e Compare July 2, 2015 19:02

thefourtheye reviewed Jul 2, 2015
View reviewed changes

thefourtheye mentioned this pull request Jul 5, 2015

path: Throwing error on zero length strings #2105

Closed

thefourtheye reviewed Jul 6, 2015
View reviewed changes

cjihrig mentioned this pull request Jul 6, 2015

fs: Fix StatWatcher to handle error codes passed in status nodejs/node-v0.x-archive#25469

Closed

brendanashworth force-pushed the new/fs-watchfile-tests branch from d9c795e to f6467b0 Compare July 10, 2015 08:53

thefourtheye reviewed Jul 10, 2015
View reviewed changes

brendanashworth closed this Jul 10, 2015

thefourtheye mentioned this pull request Jul 12, 2015

doc: note about non-existent files in watchFile #2169

Closed

Fishrock123 mentioned this pull request Jul 16, 2015

Release Proposal: v2.4.0 #2189

Merged

maboiteaspam mentioned this pull request Jul 16, 2015

fix #19 nodejs/changelog-maker#20

Merged

thefourtheye mentioned this pull request Jul 19, 2015

fs.watchFile calling listener on non-existing files #1745

Closed

ronkorving reviewed Jul 23, 2015
View reviewed changes

fs.watchFile fixes #2093

fs.watchFile fixes #2093

Conversation

brendanashworth commented Jul 2, 2015

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

thefourtheye commented Jul 2, 2015

kanongil commented Jul 2, 2015

bnoordhuis commented Jul 2, 2015

kanongil commented Jul 2, 2015

brendanashworth commented Jul 2, 2015

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

Choose a reason for hiding this comment

bnoordhuis commented Jul 2, 2015

brendanashworth commented Jul 3, 2015

thefourtheye commented Jul 3, 2015

brendanashworth commented Jul 3, 2015

bnoordhuis commented Jul 4, 2015

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

brendanashworth commented Jul 6, 2015

brendanashworth commented Jul 10, 2015

Choose a reason for hiding this comment

bnoordhuis commented Jul 10, 2015

brendanashworth commented Jul 10, 2015

thefourtheye commented Jul 10, 2015

brendanashworth commented Jul 10, 2015

thefourtheye commented Jul 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment