kqueue: don't immediately remove watches for all files in a directory on Delete event #526
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… on Delete event The problem was that that on Delete events for directories it would call Watcher.Remove() for all files in that directory too. This is fine if you call w.Remove() from the application, but if you rm -rf a directory the directory itself tends to be the first remove event (on FreeBSD at least): DELETE WRITE /tmp/xxx DELETE /tmp/xxx/a DELETE /tmp/xxx/b DELETE /tmp/xxx/c DELETE /tmp/xxx/d DELETE /tmp/xxx/e DELETE /tmp/xxx/f DELETE /tmp/xxx/g So what would happen is that the internal state for /tmp/xxx/a etc. would get removed, and when the event gets processed we no longer have access to it. Move Remove() to remove() with a flag to control removing files. If a watched directory is removed then the files will emit their own delete event, so we don't really need to do this. Fixes #514 Also: - Don't send Remove|Write events; that's never really useful: if it's gone, then it's gone. No other backends do this. - Save a stat call by checking the readdir error instead. - Don't stat every directory; this isn't really needed, and getting a write event before a delete is fine (and this didn't suppress the write even anyway). - Add test/kqueue.c; just a simple implementation to test things.
arp242
added a commit
that referenced
this pull request
Nov 16, 2022
Fix regression from #526, which would sometimes fail with something along the lines of: --- FAIL: TestWatchRm/remove_watched_directory (1.30s) helpers_test.go:384: fsnotify.sendDirectoryChangeEvents: open /tmp/TestWatchRmremove_watched_directory2055111636/001: no such file or directory fsnotify_test.go:750: have: REMOVE "/a" REMOVE "/b" REMOVE "/c" REMOVE "/d" REMOVE "/e" REMOVE "/f" REMOVE "/g" want: REMOVE "/" REMOVE "/a" REMOVE "/b" REMOVE "/c" REMOVE "/d" REMOVE "/e" REMOVE "/f" REMOVE "/g" REMOVE "/h" REMOVE "/i" REMOVE "/j" We can just always ignore a directory no longer existing; kqueue should still send the correct events.
arp242
added a commit
that referenced
this pull request
Nov 16, 2022
Fix regression from #526, which would sometimes fail with something along the lines of: --- FAIL: TestWatchRm/remove_watched_directory (1.30s) helpers_test.go:384: fsnotify.sendDirectoryChangeEvents: open /tmp/TestWatchRmremove_watched_directory2055111636/001: no such file or directory fsnotify_test.go:750: have: REMOVE "/a" REMOVE "/b" REMOVE "/c" REMOVE "/d" REMOVE "/e" REMOVE "/f" REMOVE "/g" want: REMOVE "/" REMOVE "/a" REMOVE "/b" REMOVE "/c" REMOVE "/d" REMOVE "/e" REMOVE "/f" REMOVE "/g" REMOVE "/h" REMOVE "/i" REMOVE "/j" We can just always ignore a directory no longer existing; kqueue should still send the correct events. Also noticed the error on sendFileCreatedEventIfNew() wasn't really being sent properly, so deal with that as well.
25 tasks
belimawr
added a commit
to belimawr/fsnotify
that referenced
this pull request
Sep 25, 2024
Release 1.7.0 This version of fsnotify needs Go 1.17. Additions: - illumos: add FEN backend to support illumos and Solaris. ([fsnotify#371]) - all: add `NewBufferedWatcher()` to use a buffered channel, which can be useful in cases where you can't control the kernel buffer and receive a large number of events in bursts. ([fsnotify#550], [fsnotify#572]) - all: add `AddWith()`, which is identical to `Add()` but allows passing options. ([fsnotify#521]) - windows: allow setting the ReadDirectoryChangesW() buffer size with `fsnotify.WithBufferSize()`; the default of 64K is the highest value that works on all platforms and is enough for most purposes, but in some cases a highest buffer is needed. ([fsnotify#521]) Changes and fixes: - inotify: remove watcher if a watched path is renamed ([fsnotify#518]) After a rename the reported name wasn't updated, or even an empty string. Inotify doesn't provide any good facilities to update it, so just remove the watcher. This is already how it worked on kqueue and FEN. On Windows this does work, and remains working. - windows: don't listen for file attribute changes ([fsnotify#520]) File attribute changes are sent as `FILE_ACTION_MODIFIED` by the Windows API, with no way to see if they're a file write or attribute change, so would show up as a fsnotify.Write event. This is never useful, and could result in many spurious Write events. - windows: return `ErrEventOverflow` if the buffer is full ([fsnotify#525]) Before it would merely return "short read", making it hard to detect this error. - kqueue: make sure events for all files are delivered properly when removing a watched directory ([fsnotify#526]) Previously they would get sent with `""` (empty string) or `"."` as the path name. - kqueue: don't emit spurious Create events for symbolic links ([fsnotify#524]) The link would get resolved but kqueue would "forget" it already saw the link itself, resulting on a Create for every Write event for the directory. - all: return `ErrClosed` on `Add()` when the watcher is closed ([fsnotify#516]) - other: add `Watcher.Errors` and `Watcher.Events` to the no-op `Watcher` in `backend_other.go`, making it easier to use on unsupported platforms such as WASM, AIX, etc. ([fsnotify#528]) - other: use the `backend_other.go` no-op if the `appengine` build tag is set; Google AppEngine forbids usage of the unsafe package so the inotify backend won't compile there. [fsnotify#371]: fsnotify#371 [fsnotify#516]: fsnotify#516 [fsnotify#518]: fsnotify#518 [fsnotify#520]: fsnotify#520 [fsnotify#521]: fsnotify#521 [fsnotify#524]: fsnotify#524 [fsnotify#525]: fsnotify#525 [fsnotify#526]: fsnotify#526 [fsnotify#528]: fsnotify#528 [fsnotify#537]: fsnotify#537 [fsnotify#550]: fsnotify#550 [fsnotify#572]: fsnotify#572
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem was that that on Delete events for directories it would call Watcher.Remove() for all files in that directory too. This is fine if you call w.Remove() from the application, but if you rm -rf a directory the directory itself tends to be the first remove event (on FreeBSD at least):
So what would happen is that the internal state for /tmp/xxx/a etc. would get removed, and when the event gets processed we no longer have access to it.
Move Remove() to remove() with a flag to control removing files. If a watched directory is removed then the files will emit their own delete event, so we don't really need to do this.
Fixes #514
Also:
Don't send Remove|Write events; that's never really useful: if it's gone, then it's gone. No other backends do this.
Save a stat call by checking the readdir error instead.
Don't stat every directory; this isn't really needed, and getting a write event before a delete is fine (and this didn't suppress the write even anyway).
Add test/kqueue.c; just a simple implementation to test things.