kqueue: wait without timeout #124
Conversation
fjl
force-pushed
the
kqueue-no-timeout
branch
3 times, most recently
from
2a1919d
to
baf2550
Compare
|
I cannot reproduce the failure on Travis and it seems unrelated to the PR. |
|
The tests pass on FreeBSD, too. |
|
I'll re-run Travis to see if it's being flaky. |
|
Looks like it worked this time. |
|
I found a minor issue with how the pipe is set up and fixed it. Travis still passing. |
|
Any ETA on getting this merged? |
|
Sorry, still haven't reviewed and tested it yet. |
|
I was also seeing Close() hangs as described in #89 -- at $job we are now carrying this patch on our vendor'ed copy of fsnotify, would love to see it merged. |
|
I'm not that familiar with |
As noted in fsnotify#24, reading from the kqueue won't unblock when it is closed on all platforms. This commit solves the issue by additionally watching on a pipe which is closed when Watcher is closed. The read timeout is no longer necessary.
|
rebased |
|
Is there anything I can do to get this merged? |
|
Thanks for your patience @fjl. There is currently a data race issue with kqueue on master, which should be remedied first. As to this PR, we need some familiar with epoll to do the code review. |
|
A lot of things changed since this PR was opened and it doesn't rebase cleanly with quite a few conflicts. It's not entirely clear to me if this problem has been solved since 2014 or if it still exists. Either way, I think it would be good to have a test case; I ran |
|
@arp242 GitHub Actions will run on macOS, if there is a useful test case, but not sure about running such a high count. On macOS 12.4:
|
|
Oh, you'll have to add
Yeah, just a quick low-effort way to see if intermittent problems show up; if they do, a dedicated (non-slow) test case can be added. |
|
This is the result running another time on macOS 12.5: |
|
Coolio, I can't reproduce that on FreeBSD (just tried again to be sure), but it confirms that the problem still exists, and that it's specific to macOS and not all of kqueue (probably). So need to rebase and write a decent test case which doesn't take 15 minutes, probably something similar to this. |
|
#262 seems to do the same btw, might want to go with that one as it seems a bit simpler. |
The timeout for unix.Kevent() is causing issues; every 100ms it will do a new unix.Kevent() syscall, which isn't too efficient: even if you have just one change an hour, you will still keep calling kevent() ten times per second, resulting in a needlessly high CPU usage. Without a timeout, kevent() will block indefinitely until there are some events, which is much more efficient. We can't just remove the timout however, since we can't interrupt the kevent() call on FreeBSD and NetBSD, and it will hang forever. This issue is described in more detail here: #262 (comment) To solve this we register a new kevent() with the file descriptor set to the closepipe; when this pipe is closed an event is sent and kevent() will stop blocking. This is a rebased version of #124. Fixes #89 Fixes #237 Fixes #333 Supersedes and closes #124 Supersedes and closes #262 Supersedes and closes #334
|
New version of this PR: #480 I couldn't update this PR for some reason; I guess the branch/fork disappeared after all these years? Anyway, it's essentially the same patch, but rebased on the latest main and with some minor cleanups. |
The timeout for unix.Kevent() is causing issues; every 100ms it will do a new unix.Kevent() syscall, which isn't too efficient: even if you have just one change an hour, you will still keep calling kevent() ten times per second, resulting in a needlessly high CPU usage. Without a timeout, kevent() will block indefinitely until there are some events, which is much more efficient. We can't just remove the timout however, since we can't interrupt the kevent() call on FreeBSD and NetBSD, and it will hang forever. This issue is described in more detail here: #262 (comment) To solve this we register a new kevent() with the file descriptor set to the closepipe; when this pipe is closed an event is sent and kevent() will stop blocking. This is a rebased version of #124. Fixes #89 Fixes #237 Fixes #333 Supersedes and closes #124 Supersedes and closes #262 Supersedes and closes #334 Co-authored-by: Felix Lange <fjl@twurst.com>
As noted in #24, reading from the kqueue won't unblock when
it is closed on all platforms. This commit solves the issue by
additionally watching on a pipe which is closed when Watcher
is closed. This makes the read timeout unnecessary.