Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

HandleWatcher replacement error handling logic is broken #98

Open
glasser opened this issue Dec 22, 2015 · 0 comments
Open

HandleWatcher replacement error handling logic is broken #98

glasser opened this issue Dec 22, 2015 · 0 comments

Comments

@glasser
Copy link
Contributor

glasser commented Dec 22, 2015

7e10102 changed HandleWatcher.start to "Remove the old one if we found two same file handles" instead of throwing an exception.

Choosing to replace an existing HandleWatcher with a new one with just a logged error instead of throwing an exception might make sense. But the choice in that commit to call troubleWatcher.close() seems broken.

troubleWatcher.close() does two things: binding.unwatch, and handleWatchers.remove. The latter is reasonable: we're trying to replace the old bogus HandleWatcher with the new one, so we should remove it, sure.

But binding.unwatch here is problematic: the argument we're passing to it is just the fd that we got back from binding.watch, which we're going to pass to close (Mac) or inotify_rm_watch (Linux). But... that means we're actually closing the fd we just got back from binding.watch and which is saved in the new HandleWatcher!

ie, this error case means we've suddenly learned that a HandleWatcher in handleWatchers contains an invalid WatcherHandle (fd). Going and calling close on that invalid handle is just going to break the new thing we got.

glasser referenced this issue Dec 22, 2015
Somehow sometimes the binding.watch would return a handle that was
already taken by a live handler, I have no idea of why this happened,
but we simply remove the old one for now so we do not have to throw an
exception.
benjamn pushed a commit to meteor/meteor that referenced this issue Oct 22, 2016
It's a shame that Pathwatcher issues this warning using console.error,
without taking any verbosity options into account:
https://github.com/atom/node-pathwatcher/blob/7ef76e5dfd/src/main.coffee#L53

Fortunately, I believe I've identified the underlying reason why this
happens, which may help resolve the following issue:
atom/node-pathwatcher#98

If all goes well, I'll submit an upstream pull request.

I've also reinstated an old file watching test that I mistakenly removed
when I attempted to switch to chokidar instead of pathwatcher.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant