Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: fix StatWatcher to handle error codes #2028

Conversation

brendanashworth
Copy link
Contributor

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: #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.watchFile is not covered at all by the test suite, do not rely on that for checking backwards compatibility.

@@ -0,0 +1,14 @@
'use strict';

var assert = require('assert');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the functionality is different from 0.10, I think we should at least have something in the docs regarding the difference.

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jun 22, 2015
@brendanashworth
Copy link
Contributor Author

I'm going to move ahead with #2093 instead, which keeps current functionality. Thank you everyone.

brendanashworth added a commit to brendanashworth/io.js that referenced this pull request Jul 10, 2015
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 added a commit that referenced this pull request Jul 10, 2015
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fs.watchFile calling listener on non-existing files
6 participants