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

config.js file watching broken on OSX #182

Closed
ciaranj opened this issue Oct 24, 2012 · 5 comments
Closed

config.js file watching broken on OSX #182

ciaranj opened this issue Oct 24, 2012 · 5 comments

Comments

@ciaranj
Copy link
Contributor

ciaranj commented Oct 24, 2012

commit: 19814bd appears to break OSX . The API call appears incorrect. Could be down to the version of node.js I'm using, but fs.watch takes function( event, filename_) [filename_ not present in OSX incidentally] not function( curr, prev ) as fs.watchFile does.

This means I see the following errors:

/Users/ciaran/Documents/Development/statsd/lib/config.js:25
    if (curr.ino != prev.ino) { self.updateConfig(); }
                        ^
TypeError: Cannot read property 'ino' of null
    at FSWatcher.Configurator (/Users/ciaran/Documents/Development/statsd/lib/config.js:25:25)
    at FSWatcher.EventEmitter.emit (events.js:96:17)
    at FSEvent._handle.onchange (fs.js:826:12)

Now, I would provide a patch to fix it, but since the commit that introduces the changes gives no hints as to what was being fixed, I can't make sure that it remains fixed.

If I had to guess I'd say this is broken for everyone, but on Windows & Linux the second argument (filename) does actually exist so prev.ino resolves to undefined, rather than trying to find .ino on undefined as in my case (OSX)

@jgoulah
Copy link
Contributor

jgoulah commented Oct 24, 2012

please provide the version of node.js that is installed on your system

@ciaranj
Copy link
Contributor Author

ciaranj commented Oct 24, 2012

v0.8.12, also I meant to include this link previously:

http://nodejs.org/docs/latest/api/fs.html#fs_fs_watch_filename_options_listener

(sorry)

@peschuster
Copy link

It was changed in pull request #145, because fs.watchFile is not available on windows.
But you're right. The callback function needs to be changed accordingly.

I'd suggest:

fs.watch(file, function (event, filename) {
    if (event != 'rename') { self.updateConfig(); }
});

ciaranj added a commit to ciaranj/statsd that referenced this issue Oct 28, 2012
Implements fix as suggested by Peter Schuster in comment thread.

Signed-off-by: ciaranj <ciaranj@gmail.com>
@ciaranj
Copy link
Contributor Author

ciaranj commented Oct 28, 2012

I've submitted a pull request with @peschuster ' s suggested fix.

@mrtazz
Copy link
Member

mrtazz commented Nov 3, 2012

closing this since we have pull request #184

@mrtazz mrtazz closed this as completed Nov 3, 2012
ciaranj added a commit to ciaranj/statsd that referenced this issue Nov 6, 2012
Implements fix as suggested by Peter Schuster and Daniel
Schauenberg in comment thread.

Signed-off-by: ciaranj <ciaranj@gmail.com>
mrtazz added a commit that referenced this issue Nov 10, 2012
Fixes Issues #182 - Broken config file watching.
hbouvier pushed a commit to hbouvier/statsd that referenced this issue May 25, 2013
Implements fix as suggested by Peter Schuster and Daniel
Schauenberg in comment thread.

Signed-off-by: ciaranj <ciaranj@gmail.com>
hbouvier pushed a commit to hbouvier/statsd that referenced this issue May 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants