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: don't immediately remove watches for all files in a directory on Delete event #526

Merged
merged 1 commit into from Oct 15, 2022

Conversation

arp242
Copy link
Member

@arp242 arp242 commented Oct 15, 2022

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.

… 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 arp242 merged commit 8f6708d into main Oct 15, 2022
@arp242 arp242 deleted the kq-rmdir branch October 15, 2022 08:33
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.
@shogo82148 shogo82148 mentioned this pull request Mar 6, 2024
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On MacOS, removing watched directory produces series of wrong create/remove events
1 participant