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

recursive watch on large directory exhausting inotifywatch does not cause error #109

Open
adamjacobmuller opened this issue Oct 6, 2016 · 5 comments
Labels

Comments

@adamjacobmuller
Copy link

Trivial repro but including it anyway.
`
package main

import (
"log"

"github.com/rjeczalik/notify"

)

func main() {
c := make(chan notify.EventInfo, 1)

if err := notify.Watch("/home/adam/...", c, notify.All); err != nil {
    log.Fatal(err)
}
defer notify.Stop(c)
for {
    ei := <-c
    log.Println("event", ei)
}

}
`

/home/adam/ is a directory with a lot (probably a few million) files.

In this case, at some point during setting up the notify.Watch it runs out of user events but err is not set.

@adamjacobmuller
Copy link
Author

would there be any support for an fanotify patch? It doesn't include some of the granularity of inotify but it also doesn't suffer from this issue

@rjeczalik
Copy link
Owner

@adamjacobmuller Thanks for the report. The ideal solution would be here to either fallback to polling or keep retrying setting the watch with some backoff.

Currently notify does not have an API to return watch errors (with a channel of some sort), as the philosophy was "if notify is not able to recover from watch error then user also won't be able to". After working for a while with such approach I understood it is limiting and something that I'd definitely want to change by introducing new API (backward-compatible one). For now there is not easy workaround for that.

would there be any support for an fanotify patch?

I'm not sure how it could work out, fanotify does not support notify.Create and notify.Remove events, the ones that are expected to be supported by all the platforms. It would be ok to have it as a opt-in target for Linux, so the users are aware they are switching to limited implementation.

@rjeczalik rjeczalik added the bug label Oct 7, 2016
@bacongobbler
Copy link

bacongobbler commented Feb 24, 2017

Just wanted to bump this issue as well. I was scratching my head for a few hours on why inotify events were not being seen by this lib and it turned out I too ran out of watches, however I was out of watches before it was initiated.

On debian:

$ sudo apt-get install inotify-tools
$ inotifywait -rme modify,attrib,move,close_write,create,delete,delete_self /tmp
Setting up watches.  Beware: since -r was given, this may take a while!
Failed to watch /tmp; upper limit on inotify watches reached!
Please increase the amount of inotify watches allowed per user via `/proc/sys/fs/inotify/max_user_watches'.

Bumping the default 8192 to 16384 in /proc/sys/fs/inotify/max_user_watches did the trick!

I'm going to see if there's a way we can return watch errors on another channel as @rjeczalik mentioned.

@bacongobbler
Copy link

bacongobbler commented Feb 24, 2017

Also for the record, if I did not watch a directory recursively (i.e. no ... on the notify path), it would fail with the somewhat cryptic No space left on device error, which seems to be how other programs like tail would fail as well. If it's a recursive watch, the error is not raised and we end up where I began my debugging.

@nekohayo
Copy link

Providing an update on this:

I'm not sure how it could work out, fanotify does not support notify.Create and notify.Remove events, the ones that are expected to be supported by all the platforms. It would be ok to have it as a opt-in target for Linux, so the users are aware they are switching to limited implementation.

It seems the situation has changed, as I discovered in https://gitlab.gnome.org/GNOME/localsearch/-/issues/109 :

Since Linux 5.1 the FANotify API supports create, move and delete events. This means we could finally use FANotify in place of inotify.

This is corroborated by @infectormp's report in syncthing/syncthing#5755

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

4 participants