rjeczalik / notify

File system event notification library on steroids.
MIT License
907 stars 130 forks source link

Silent fail when exceeding inotify watch limits (Linux) #76

Open Zillode opened 9 years ago

Zillode commented 9 years ago

Currently, no errors are raised when exceeding the /proc/sys/fs/inotify/max_user_watches limit on Linux (I guess this is also true for OSX). Therefore, our client application cannot know whether all watches are properly installed, which makes all benefits fade away. Is there a way to capture incomplete inotify handlers?

ppknap commented 9 years ago

hi @Zillode, thank you for reporting this to us.

I will fix this issue as soon as possible (probably tomorrow).

Regarding incomplete inotify handlers, at this moment the only solution is to increment the number of maximum user watchers by modifying the file you mentioned above. However, there are(were) plans to switch to polling when something goes wrong with native watchers, but it is @rjeczalik who can say more about this as well as if the problem occurs on OSX platform.

rjeczalik commented 9 years ago

(I guess this is also true for OSX)

@Zillode It's not, it uses FSEvents and the implementation is able to minimize number of watches for multiple paths by watching their earliest ancestor. The problem with fd limits is real for kqueue.

By implementing polling to dodge one limit we will fall into another one - fd limit, however it may be a bit benigner. What I think could improve the situation here are watchpoint filters - e.g. if you're watching $HOME recursively you could filter out directory trees starting at .git, .svn, .hg and other directories that you might not be interested in, so we spare watches on those. However we have not thought about possible API, which could be nice and mutli-platform. Opinions?

BTW excuse me chiming in, but since you have tests for Windows I believe you aim for multi-platform support - here you probably want to use filepath.Join.

Zillode commented 9 years ago

Thanks for the quick responses!

It is true we aim for cross-platform support, and I'll fix the filepath.Join issue you mentioned. It's also nice to see that this library fixes major issues we've had with go-fsnotify/fsnotify on OSX. However, OSX is the only platform which is incompatible with cross-compilation, correct?

The current Syncthing behaviour uses polling and I think it will stay like this to accommodate for lost messages from inotify/fsevents/... and to support the Rest API. Therefore I don't think we'll switch to an external polling feature.

Implementing ignores would be interesting for us. With the go-fs library, we ignored certain directories using strings and regexs while installing the notify handlers. However, I think this won't fix the problem for all users, given that a lot of them sync many files.

rjeczalik commented 9 years ago

However, OSX is the only platform which is incompatible with cross-compilation

@Zillode No and yes. No, you can't cross-compile CGO code with vanilla go toolchain not having cross-compilers set up (CC_FOR_TARGET etc.). Yes, you can however pre-built dependencies only once on Darwin, and use compiled packages for cross-compiling under Linux. You install notify on Darwin with go install github.com/rjeczalik/notify and copy $GOPATH/pkg/pkg/darwin_amd64/github.com/rjeczalik/notify.a (and possible another one for darwin_amd64_race) over to your linux box (it can be nicely automated with some artefact system). Basically the same you do when you want to have stdlib for cross-compilation with CGO DNS and os/user support for all platforms, you just grab archives from golang.org/dl and unzip its lib/$PLATFORM files to your GOROOT.

When I have spare 5 minutes I'll grab your soft from your experiment branch and see the problem myself, maybe there's something more than can be done.

[edit] The error handling is not yet fully done in notify, we want to gracefully handle them, e.g. propagate errors internally and retry setting watchpoints with some backoffs. There's lots of TODOs about it in the code, we'd simply prioritised functionality first, resilience second.

nathany commented 9 years ago

@rjeczalik If enabling cross-compilation with an OS X target, would it make sense to have a thin wrapper around FSEvents that rarely changes, and is then used by notify (or fsnotify)?

rjeczalik commented 9 years ago

@nathany Do you mean something like FSEvents-bindings for Go? It could be doable to turn watcher_fsevents_cgo.go into some notify/fsevents package, however there's a lot of hardcoded parameters, so some refactoring would be required.

From the cross-compilation viewpoint nothing will change, you still would need to keep fsevents.a compiled on Darwin around. Actually making the library fine-grained could increase a hussle for users, e.g. assuming we have FEN for Solaris, which also uses CGO, users would need to keep two compiled package files - fsevents.a and fen.a if we go that route. So little gain for cross-compilation.

nathany commented 9 years ago

That's a good point regarding cross-compilation. So maybe notify.a is better. To avoid further hijacking this issue, I opened a new issue regarding low-level bindings. https://github.com/go-fsnotify/fsnotify/issues/75

Zillode commented 9 years ago

Will https://github.com/rjeczalik/notify/commit/00b250008103ba9e5d8d4f126327d0edf48253f9 be merged in master?

ppknap commented 9 years ago

No, this commit is a workaround which doesn't unwatch already watched files when the file descriptor limit is reached. Since we want recursive watchers to act like a transaction, we decided to not marge this into a master branch.

As a workaround, you may fork the repository and cherry-pick https://github.com/rjeczalik/notify/commit/00b250008103ba9e5d8d4f126327d0edf48253f9 to your master.