rjeczalik / notify

File system event notification library on steroids.
MIT License
906 stars 128 forks source link

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

Open adamjacobmuller opened 8 years ago

adamjacobmuller commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

bacongobbler commented 7 years ago

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 commented 7 years ago

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 commented 4 months ago

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 https://github.com/syncthing/syncthing/issues/5755