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

Reflex does not remove file watches #13

Open
aktau opened this issue Jul 8, 2014 · 4 comments
Open

Reflex does not remove file watches #13

aktau opened this issue Jul 8, 2014 · 4 comments
Assignees
Labels

Comments

@aktau
Copy link

aktau commented Jul 8, 2014

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
Copy link
Owner

cespare commented Jul 8, 2014

Thanks, I'll look into it.

@cespare cespare changed the title Removing the watch is perhaps not correct Reflex does not remove file watches Sep 12, 2014
@cespare
Copy link
Owner

cespare commented Sep 12, 2014

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

cespare added a commit that referenced this issue Sep 12, 2014
@cespare cespare added the bug label Sep 13, 2014
@cespare cespare self-assigned this Nov 22, 2014
@pkieltyka
Copy link

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
Copy link
Owner

cespare commented Jan 12, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants