cespare / reflex

Run a command when files change
MIT License
3.35k stars 135 forks source link

Reflex does not remove file watches #13

Open aktau opened 10 years ago

aktau commented 10 years ago

While reading of the code I noticed:

func watch(...) { 
...
        case e := <-watcher.Event:
            path := strings.TrimPrefix(e.Name, "./")
            if e.IsDelete() {
                watcher.RemoveWatch(path)
            }
...
}

This might not be entirely correct. RemoveWatch checks if path exists in a map. Because it's been trimmed earlier (TrimPrefix), it likely will not find it and just return an error. I believe watcher.RemoveWatch(e.Name) would have a better chance of succeeding. Also perhaps checking the returned error. An error will be returned on many occasions though, because if path is not a folder, a watch was never issued for it. So RemoveWatch will probably fail for those cases.

I'm also not sure how some platforms handle it, but deleting a file might automatically delete the associated watcher (if any) on the kernel side. Calling RemoveWatch could still be beneficial though, to clean up the structure on the Go side.

cespare commented 10 years ago

Thanks, I'll look into it.

cespare commented 9 years ago

@aktau Sorry for the long delay.

You may be correct; I haven't looked at older fsnotify code. The newer code, at least, cleans the paths, so this wouldn't be a problem.

I'm updating to fsnotify.v1 now, in any case, so this won't be an issue.

However...

I realized that the removal code is not at all correct. For one thing, it does not recursively remove the watches. For another, you can't even remove watches in fsnotify for files that have been deleted :( I've filed go-fsnotify/fsnotify#40 and go-fsnotify/fsnotify#41. So for right now, it's not even possible to remove watches.

I'm removing the watch removal code completely for now. Hopefully I can soon find a workaround or patch fsnotify to make removing recursive watches possible.

I've edited the title to reflect the current status of this issue.

pkieltyka commented 9 years ago

I'm not sure if this is related, but I do find reflex to be progressively slower over time of use.. and starts to happen after about 10 runs of reflex to rerun go test

cespare commented 9 years ago

Hi @pkieltyka! Can you describe what you mean by slower? Does it just take longer to respond to file changes? Can you give your exact repro case (what reflex command you're running and what filesystem changes you're making)?

Note that this bug would only be related if you're adding and removing directories.

It might be best if you opened a new issue here with the repro info.