kqueue: synchronously close fd in Close() #334

dottedmag · 2020-04-20T17:09:22Z

This makes it possible to create and destroy watchers quickly on macOS (e.g. in
unit tests).

Closes #333

This makes it possible to run and destroy watchers quickly on macOS (e.g. in unit tests). Closes #333

dottedmag · 2020-04-20T21:35:52Z

Tests breakage is pre-existing: #264

dottedmag · 2020-05-27T10:31:38Z

Ping?

crawshaw · 2020-05-31T03:40:57Z

We use a resource checker in our code that counts total process goroutines before and after, and we are seeing occasionally a goroutine left on macOS after Close is called.

This PR is a great improvement. Thanks.

(One thing I would consider adding would be to make Close wait for the readEvents goroutine to complete so we can ensure all resources are cleaned up before it completes, but regardless this PR makes things better.)

feldgendler · 2020-08-17T11:53:21Z

@crawshaw Hi! Since you think this PR is an improvement despite occasional pre-existing test failures, may we ask you to merge it?

crawshaw · 2020-08-18T04:54:05Z

@feldgendler this isn't my repository

feldgendler · 2021-02-09T13:24:48Z

@nathany May I ask you for a review?

arp242 · 2022-08-01T20:28:29Z

The tests for FreeBSD and OpenBSD fail:

FreeBSD: https://github.com/fsnotify/fsnotify/runs/7619130055?check_suite_focus=true

--- FAIL: TestWatch (0.00s)
    --- FAIL: TestWatch/multiple_creates (1.19s)
        helpers_test.go:33: 
            have:
                
            want:
                CREATE               "/file"
                WRITE                "/file"
                REMOVE               "/file"
                CREATE               "/file"
                WRITE                "/file"
                WRITE                "/file"
--- FAIL: TestWatchRename (0.00s)
    --- FAIL: TestWatchRename/rename_file (0.87s)
        helpers_test.go:33: 
            have:
                
            want:
                CREATE               "/file"
                WRITE                "/file"
                RENAME               "/file"
                CREATE               "/renamed"
    --- FAIL: TestWatchRename/rename_to_unwatched_directory (0.93s)
        helpers_test.go:33: 
            have:
                CREATE               "/file"
                WRITE                "/file"
            want:
                CREATE               "/file"
                WRITE                "/file"
                RENAME               "/file"
                CREATE               "/file"
--- FAIL: TestWatchSymlink (0.00s)
    --- FAIL: TestWatchSymlink/cyclic_symlink (0.76s)
        helpers_test.go:33: 
            have:
                
            want:
                WRITE                "/link"
                CREATE               "/link"
--- FAIL: TestWatchAttrib (0.00s)
    --- FAIL: TestWatchAttrib/chmod (0.74s)
        helpers_test.go:33: 
            have:
                
            want:
                CHMOD                "/file"
FAIL
FAIL	github.com/fsnotify/fsnotify	8.203s
?   	github.com/fsnotify/fsnotify/cmd/fsnotify	[no test files]
FAIL

OpenBSD: https://github.com/fsnotify/fsnotify/runs/7619131191?check_suite_focus=true

--- FAIL: TestWatch (0.00s)
    --- FAIL: TestWatch/multiple_creates (1.20s)
        helpers_test.go:33: 
            have:
                
            want:
                CREATE               "/file"
                WRITE                "/file"
                REMOVE               "/file"
                CREATE               "/file"
                WRITE                "/file"
                WRITE                "/file"
FAIL
FAIL	github.com/fsnotify/fsnotify	33.606s
?   	github.com/fsnotify/fsnotify/cmd/fsnotify	[no test files]
FAIL

If I run it locally, the results are somewhat inconsistent: it doesn't always fail and doesn't always fail in the same way. I've also seen it fail locally with NetBSD:

[~cg/fsnotify](333-macos-wait-kqueue-fd-close)% goon netbsd test
goon: netbsd already started; nothing to do
PASS
ok      github.com/fsnotify/fsnotify    25.654s

[~cg/fsnotify](333-macos-wait-kqueue-fd-close)% goon netbsd test
goon: netbsd already started; nothing to do
--- FAIL: TestWatch (0.00s)
    --- FAIL: TestWatch/multiple_creates (1.17s)
        helpers_test.go:33:
            have:

            want:
                CREATE               "/file"
                WRITE                "/file"
                REMOVE               "/file"
                CREATE               "/file"
                WRITE                "/file"
                WRITE                "/file"
FAIL
exit status 1
FAIL    github.com/fsnotify/fsnotify    24.013s

The first run was fine, but in the second there were no events at all(!)

Same on macOS:

[~cg/fsnotify](333-macos-wait-kqueue-fd-close)% goon macos test
goon: starting QEMU VM from macos-12.qcow2
goon: waiting for ssh to be available at localhost:9924 … Okay
--- FAIL: TestWatch (0.00s)
    --- FAIL: TestWatch/multiple_creates (1.18s)
        helpers_test.go:33:
            have:

            want:
                CREATE               "/file"
                WRITE                "/file"
                REMOVE               "/file"
                CREATE               "/file"
                WRITE                "/file"
                WRITE                "/file"
FAIL
exit status 1
FAIL    github.com/fsnotify/fsnotify    8.400s

I didn't investigate why this is happening yet, but I my guess is that it's returning before its finished reading(?)

Also note the OpenBSD test takes forever; mostly due to that TestOpenCloseQuickly test that was added, but that's a minor (fixable) issue.

arp242 · 2022-08-01T20:29:12Z

Note that the above is after I rebased the branch on main, which is over here: https://github.com/fsnotify/fsnotify/tree/333-macos-wait-kqueue-fd-close

arp242 · 2022-08-01T20:47:52Z

#124 seems to fix the same problem, and all tests pass on that one. Updated branch here, but needs some more work as it's a pretty old branch: https://github.com/fsnotify/fsnotify/tree/kqueue-no-timeout

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

arp242 · 2022-08-01T21:47:22Z

I created #480, which should solve this issue better.

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>

kqueue: synchronously close fd in Close()

15be759

This makes it possible to run and destroy watchers quickly on macOS (e.g. in unit tests). Closes #333

arp242 added macOS freebsd labels Jul 31, 2022

arp242 added the need-feedback Requires feedback to be actionable label Aug 1, 2022

arp242 mentioned this pull request Aug 1, 2022

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

Merged

arp242 closed this Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kqueue: synchronously close fd in Close() #334

kqueue: synchronously close fd in Close() #334

dottedmag commented Apr 20, 2020 •

edited

dottedmag commented Apr 20, 2020 •

edited

dottedmag commented May 27, 2020

crawshaw commented May 31, 2020

feldgendler commented Aug 17, 2020

crawshaw commented Aug 18, 2020

feldgendler commented Feb 9, 2021

arp242 commented Aug 1, 2022 •

edited

arp242 commented Aug 1, 2022 •

edited

arp242 commented Aug 1, 2022

arp242 commented Aug 1, 2022

kqueue: synchronously close fd in Close() #334

kqueue: synchronously close fd in Close() #334

Conversation

dottedmag commented Apr 20, 2020 • edited

dottedmag commented Apr 20, 2020 • edited

dottedmag commented May 27, 2020

crawshaw commented May 31, 2020

feldgendler commented Aug 17, 2020

crawshaw commented Aug 18, 2020

feldgendler commented Feb 9, 2021

arp242 commented Aug 1, 2022 • edited

arp242 commented Aug 1, 2022 • edited

arp242 commented Aug 1, 2022

arp242 commented Aug 1, 2022

dottedmag commented Apr 20, 2020 •

edited

dottedmag commented Apr 20, 2020 •

edited

arp242 commented Aug 1, 2022 •

edited

arp242 commented Aug 1, 2022 •

edited