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

kqueue: remove timeout from unix.Kevent() #480

Merged
merged 5 commits into from Aug 6, 2022
Merged

kqueue: remove timeout from unix.Kevent() #480

merged 5 commits into from Aug 6, 2022

Conversation

arp242
Copy link
Member

@arp242 arp242 commented Aug 1, 2022

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

fjl and others added 4 commits April 21, 2016 11:12
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. The read timeout is no longer necessary.
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
@dottedmag
Copy link

Please add TestOpenCloseQuickly from #334. I have checked that on macOS 12.4 it still fails on master, and passes on this branch.

Updated test:

// closing a watcher should synchronously close watch file descriptor
func TestOpenCloseQuickly(t *testing.T) {
	tmp := t.TempDir()
	name := filepath.Join(tmp, "testfile")

	touch(t, name)

	for i := 0; i < 10000; i++ {
		// if file descriptors are not closed synchronously
		// this will fail with "too many open files"
		w := newWatcher(t)
		err := w.Add(name)
		if err != nil {
			t.Fatal(err)
		}
		err = w.Close()
		if err != nil {
			t.Fatal(err)
		}
	}
}

@arp242
Copy link
Member Author

arp242 commented Aug 2, 2022

Please add TestOpenCloseQuickly from #334.

Thanks; I looked and your test and it was useful to verify this PR; I added it in a local test-resource-usage branch that I've been working on and also tests some other things, but I haven't finished/pushed that yet.

The problem is that it takes a very long time to run on various platforms: ~25 seconds on Linux, ~40 seconds on OpenBSD (because OpenBSD is slow), etc. which is quite long given that the test of the test suite runs in ~7 seconds. Also it doesn't really test anything except by "brute force". I'm pretty sure we can write a better test for this, but need to look at this and I don't think it's needed right now.

@dottedmag
Copy link

I'm fine with it being Darwin-specific for now. On macOS it passes in ~500ms.

@fjl
Copy link
Contributor

fjl commented Aug 3, 2022

6 years in the making... I'm interested to see if this PR will finally be merged :)

@arp242
Copy link
Member Author

arp242 commented Aug 3, 2022

6 years in the making... I'm interested to see if this PR will finally be merged :)

Yes, it will. The only reason it's not yet is because I wanted to give other people (e.g. fsnotify members) a chance to review it, but I'll merge it after the weekend regardless if no one does.

Going forward, PRs should be reviewed and merged (or rejected if it's no good) in a reasonable timeframe (days or weeks, rather than years).

@arp242 arp242 merged commit 4b43fad into main Aug 6, 2022
@arp242 arp242 deleted the kqueue-no-timeout branch August 6, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants