fs: fix StatWatcher to handle error codes #2028
Conversation
| @@ -0,0 +1,14 @@ | |||
| 'use strict'; | |||
|
|
|||
| var assert = require('assert'); | |||
There was a problem hiding this comment.
Thanks, fixed!
Previously, fs.watchFile() would call the callback even if the file does not exist. This is erroneous and is fixed by correctly handling the error code provided. Ref: nodejs/node-v0.x-archive#25469 Fixes: nodejs#1745
brendanashworth
force-pushed
the
fix/fs-watch-nonexistent-file
branch
from
28b3b30
to
34bd27c
Compare
| var filename = '/path/to/file/that/does/not/exist'; | ||
|
|
||
| fs.watchFile(filename, function() { | ||
| throw new Error('callback should not be called for non-existent files'); |
There was a problem hiding this comment.
@thefourtheye pointed out in an earlier revision that this means that the callback won't get called on ENOENT.
I think that already happens if you watch e.g. /etc/shadow because that's going to fail with EPERM or -1, passing the newStatus === -1 check in fs.watchFile().
That does seem unfortunate, though. Invoking the callback at least gives the user a chance to handle the ENOENT case because then you can check that curr.ino > 0.
There was a problem hiding this comment.
@bnoordhuis Yup, if the callback is not fired then the users will not know if the code is working or not. This could be a debugging nightmare.
There was a problem hiding this comment.
We can leave this in and mark the current functionality as correct. I marked the issue as confirmed-bug because folks in joyent/node did and ported the PR - but I don't have a strong opinion on whether or not the callback should be called.
There was a problem hiding this comment.
If the functionality is different from 0.10, I think we should at least have something in the docs regarding the difference.
|
I'm going to move ahead with #2093 instead, which keeps current functionality. Thank you everyone. |
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
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 <info@bnoordhuis.nl> PR-URL: #2093
Previously,
fs.watchFile()would call the callback even if the file doesnot exist. This is erroneous and is fixed by correctly handling the
error code provided.
Ref: nodejs/node-v0.x-archive#25469
Fixes: #1745
CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/65/
I just grabbed the commit from joyent/node and doctored it up to fit the new code guidelines for io.js (just the test), squashed and redid the commit log. A note to reviewers:
fs.watchFileis not covered at all by the test suite, do not rely on that for checking backwards compatibility.