fs.watchFile inconsistent stat return

[dragosnicolae]

• Version: tested on v8.4.0 and v6.11.2
• Platform: GNU/Linux (Centos 7)

Hi!

I encountered the following problem when using fs.watchFile. I start watching a file, let's say file1, the file is renamed into file2 and then back into file1.

On the first rename, my listener is called with currentStat filled with 0s (correct, as the file does not exist) and some values for the previousStat.

On the second rename, when the file gets the name it had initially, the previousStat is the same one as the previousStat of the first rename (I think it should be the one filled with 0s, as the file did not exist previously) and has the same modified time as the currentStat. This way, I cannot determine that the file has reappeared.

I am comparing that the modified time has changed in order to avoid false alarms triggered by file access as specified in the Node.js documentation.

[bnoordhuis]

The node.js documentation doesn't mention it but that's the correct and expected behavior for uv_fs_poll_start(), the libuv function that fs.watchFile() uses under the hood.

If the mtime is the same, the file was moved rather than created anew. If you get a callback with an all-fields-zero stats object, the next callback is a signal that the file has reappeared.

I'll add documentation labels.

[dragosnicolae]

Thank you for the quick response.

[niveditn]

Hey @bnoordhuis, I can pick this up if that's okay.

Through what I understand from the discussion above and my own examination of the behavior mentioned, I need to add a Note at the bottom of this section, is that correct?

Please let me know if there is anything I need to keep in mind. Meanwhile, I'll get started on this.

Thanks!

[acarstoiu]

The expected behaviour of fs.watchFile() is to hide state processing, not to increase the burden on the programmer who otherwise has to keep track of previously reported zero-ed states.

When a watched file reappears either because it was deleted and recreated or simply renamed twice (back and forth), the previous stat object should definitely be zero-ed.

Just try putting yourself in the shoes of someone supervising the file. The only certainty stated by documentation is

To be notified when the file was modified, not just accessed, it is necessary to compare curr.mtime and prev.mtime.

All other properties are uncertain, so this is the only real check that the programmer can make. But the way fs.watchFile() returns now the previous stat object in the said cases makes the check miss the change. 🔕

[acarstoiu]

➡️ Also mind that fs.watchFile() is watching paths as opposed to inodes, watched by fs.watch() (which throws if the given path does not resolve to an existing file/directory).

[bnoordhuis]

@niveditn Yes, that's right.

as opposed to inodes, watched by fs.watch()

@acarstoiu fs.watch( ) doesn't necessarily watch inodes, it depends on the platform.

[acarstoiu]

Doesn't matter, the discussion was about fs.watchFile().

[bnoordhuis]

Yes... you were the one who brought up fs.watch(), remember?

[acarstoiu]

I'll say it more clearly: read and comment my arguments against your dismissal of this ticket as a documentation bug (when in fact it's an implementation issue).
Do not steer the discussion sideways, please.

[bnoordhuis]

I've outlined why it's working as expected per the current implementation, which goes back to node.js v0.6 and behaved mostly identical even before that.

Your comment starts off with "The expected behaviour of fs.watchFile() is..." when in fact that's merely your expectation.

[acarstoiu]

By "expected" I meant what is presumable, sane and efficient. If you deem the legacy behaviour as being "expected" in all cases, then you're effectively denying improvements.
(Off-topic: it's like the two meanings of "normal" - what is frequently done vs. what the rules/norms indicate that should be done)

Also, this doesn't seem at all an awfully breaking change. In fact, nothing bad happens with the code implemented as advised by you (i.e. retain state also in client code), it will just be more suboptimal than it is right now.

[bnoordhuis]

Undocumented doesn't mean unused. fs.watchFile() is used a lot and has behaved this way a long time. Any change will almost certainly break someone's code, and for what exactly?

If you still think your suggestion is a good idea, open a pull request and argue your case.