Sure thing, I'll take a look at this tonight or tomorrow. Thanks.

Don't have much time (I'm on holiday!) but I've got a few minutes.

Sorry, I don't have a ton of time :(

I'm a bit confused about what the patch is meant to fix. Could you elaborate?

Thank you for taking your time.
I will restore commens. How about changing first comment:
inotify_rm_watch will return EINVAL if the file has been deleted;
the inotify will already have been removed.
watches and pathes are deleted in ignoreLinux() implicitly and asynchronously
by calling inotify_rm_watch() below. e.g. readEvents() goroutine receives IN_IGNORE
so that EINVAL means that the wd is being rm_watch()ed or its file removed
by another thread and we have not received IN_IGNORE event.

Thanks. There was some stuff in there I thought might save some time looking stuff up in the future :)

LGTM. I don't have a linux machine handy so I can't play around with it, but it looks good.

Thanks @chamaken. I can take a look sometime over the next few days.

Sorry, I still haven't checked this. :-(

Merged. Thanks.

This change e253224 is making some of my unit tests fail non-deterministically. After adding some tracing to the code, here's what I'm seeing (both on Linux 3.4.43 and 4.1.18):

When Remove() is called on a Watcher, it now Wait()s on a condition variable after calling syscall.InotifyRmWatch(). The condition variable is notified from ignoreLinux(), at the point that we see an Event with the IN_IGNORED bit set. This seems reasonable as the man page for inotify_rm_watch says:

Removing a watch causes an IN_IGNORED event to be generated for this watch descriptor.

However I'm seeing that in two of my unit tests sometimes (more than 50% of the time) there is no IN_IGNORED event that follows the call to syscall.InotifyRmWatch(). I verified that syscall.InotifyRmWatch() doesn't return any error. There is just no event on the watch descriptor that was passed to syscall.InotifyRmWatch().

Has anybody seen this before?

OK I think I got it... With this change it's no longer safe to consume Events from the same goroutine that calls Remove(). In our case our code basically looks this:

func (shared *InotifyTracker) run() {
    // ...

    for {
        select {
        case winfo := <-shared.watch:
            shared.error <- shared.addWatch(winfo.fname)

        case fname := <-shared.remove:
            shared.removeWatch(fname) // <-------- no longer legit

        // ...
    }
}

Removing the watch leads to a deadlock: we're trying to call Remove() and Remove() is waiting to be notified that the IN_IGNORED event was received, but the IN_IGNORED event can't be sent because fsnotify is trying to write another event to the Watch.

Thanks for investigating this @tsuna. Would you mind opening a new issue so this doesn't get lost?

@nathany I'm happy to open an issue if you deem that this new code creates an undesirable side effect that needs to be fixed. As you can see above, I've submitted a PR to workaround this quirk in the tail library. But I agree that the new behavior of fsnotify as a result of this change is unfortunate because it will be more easily prone to deadlocks, depending on how the client code is written.

Yes. I think we should take another look at this code change. Adding and removing watches from the same goroutine should not cause deadlocks.

I filed bug #123. Thanks.