From 5543647517a8a85670ed6cc4f294557f637ca56d Mon Sep 17 00:00:00 2001 From: Martin Tournoij Date: Thu, 13 Oct 2022 23:48:02 +0200 Subject: [PATCH] inotify: remove watch when renaming a watched path Renaming a watched path is problematic; on inotify we just get a IN_MOVED_SELF event with the old filename and that's it; no more events for you! So if you do: watch one mv one two cat asd >two You still continue to get events for the file "one", even though it's now named "two" (the file descriptor doesn't care about the rename). There is no way we can know the new event as far as I can tell, inotifywait(1) also behaves like this. So instead of continuing in a semi-broken state just remove the watcher, like we do for deletes. On kqueue and FEN the situation is similar, and we actually already removed watchers on renames. On Windows this all works nicely; the watch is preserved and the filename is updated. I decided to keep this as-is for now, even though it's inconsistent. We actually fixed the Windows behaviour for the 1.6.0 release in #370 , so people do seem to care about it and use it, and experience with the symlink change in 1.5.0 shows it's better to keep inconsistent behaviour rather than change it. Fixing this up is something for a v2. Fixes #172 Fixes #503 --- CHANGELOG.md | 9 +++++++ backend_fen.go | 8 +++--- backend_inotify.go | 47 ++++++++++++++++++----------------- backend_kqueue.go | 12 ++++----- backend_other.go | 8 +++--- backend_windows.go | 8 +++--- fsnotify_test.go | 61 ++++++++++++---------------------------------- mkdoc.zsh | 8 +++--- 8 files changed, 72 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea60130d..f9bf9ec9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,10 +13,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changes and fixes +- inotify: remove watcher if a watched path is renamed ([#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. + - all: return ErrClosed on Add() when the watcher is closed ([#516]) [#371]: https://github.com/fsnotify/fsnotify/pull/371 [#516]: https://github.com/fsnotify/fsnotify/pull/516 +[#518]: https://github.com/fsnotify/fsnotify/pull/518 ## [1.6.0] - 2022-10-13 diff --git a/backend_fen.go b/backend_fen.go index 47372e5a..8db297ad 100644 --- a/backend_fen.go +++ b/backend_fen.go @@ -187,11 +187,11 @@ func (w *Watcher) Close() error { // // A path can only be watched once; attempting to watch it more than once will // return an error. Paths that do not yet exist on the filesystem cannot be -// added. A watch will be automatically removed if the path is deleted. +// added. // -// A path will remain watched if it gets renamed to somewhere else on the same -// filesystem, but the monitor will get removed if the path gets deleted and -// re-created, or if it's moved to a different filesystem. +// A watch will be automatically removed if the watched path is deleted or +// renamed. The exception is the Windows backend, which doesn't remove the +// watcher on renames. // // Notifications on network filesystems (NFS, SMB, FUSE, etc.) or special // filesystems (/proc, /sys, etc.) generally don't work. diff --git a/backend_inotify.go b/backend_inotify.go index 75736ce7..355729a0 100644 --- a/backend_inotify.go +++ b/backend_inotify.go @@ -120,8 +120,8 @@ type Watcher struct { fd int mu sync.Mutex // Map access inotifyFile *os.File - watches map[string]*watch // Map of inotify watches (key: path) - paths map[int]string // Map of watched paths (key: watch descriptor) + watches map[string]*watch // Map of inotify watches (path → watch) + paths map[int]string // Map of watched paths (watch descriptor → path) done chan struct{} // Channel for sending a "quit message" to the reader goroutine doneResp chan struct{} // Channel to respond to Close } @@ -209,11 +209,11 @@ func (w *Watcher) Close() error { // // A path can only be watched once; attempting to watch it more than once will // return an error. Paths that do not yet exist on the filesystem cannot be -// added. A watch will be automatically removed if the path is deleted. +// added. // -// A path will remain watched if it gets renamed to somewhere else on the same -// filesystem, but the monitor will get removed if the path gets deleted and -// re-created, or if it's moved to a different filesystem. +// A watch will be automatically removed if the watched path is deleted or +// renamed. The exception is the Windows backend, which doesn't remove the +// watcher on renames. // // Notifications on network filesystems (NFS, SMB, FUSE, etc.) or special // filesystems (/proc, /sys, etc.) generally don't work. @@ -285,25 +285,20 @@ func (w *Watcher) Remove(name string) error { // Fetch the watch. w.mu.Lock() defer w.mu.Unlock() - watch, ok := w.watches[name] - // Remove it from inotify. + watch, ok := w.watches[name] if !ok { return fmt.Errorf("%w: %s", ErrNonExistentWatch, name) } - // We successfully removed the watch if InotifyRmWatch doesn't return an - // error, we need to clean up our internal state to ensure it matches - // inotify's kernel state. + return w.remove(name, watch) +} + +// Unlocked! +func (w *Watcher) remove(name string, watch *watch) error { delete(w.paths, int(watch.wd)) delete(w.watches, name) - // inotify_rm_watch will return EINVAL if the file has been deleted; - // the inotify will already have been removed. - // watches and pathes are deleted in ignoreLinux() implicitly and asynchronously - // by calling inotify_rm_watch() below. e.g. readEvents() goroutine receives IN_IGNORE - // so that EINVAL means that the wd is being rm_watch()ed or its file removed - // by another thread and we have not received IN_IGNORE event. success, errno := unix.InotifyRmWatch(w.fd, watch.wd) if success == -1 { // TODO: Perhaps it's not helpful to return an error here in every case; @@ -318,7 +313,6 @@ func (w *Watcher) Remove(name string) error { // are watching is deleted. return errno } - return nil } @@ -417,14 +411,23 @@ func (w *Watcher) readEvents() { // the "paths" map. w.mu.Lock() name, ok := w.paths[int(raw.Wd)] - // IN_DELETE_SELF occurs when the file/directory being watched is removed. - // This is a sign to clean up the maps, otherwise we are no longer in sync - // with the inotify kernel state which has already deleted the watch - // automatically. + // inotify will automatically remove the watch on deletes; just need + // to clean our state here. if ok && mask&unix.IN_DELETE_SELF == unix.IN_DELETE_SELF { delete(w.paths, int(raw.Wd)) delete(w.watches, name) } + // We can't really update the state when a watched path is moved; + // only IN_MOVE_SELF is sent and not IN_MOVED_{FROM,TO}. So remove + // the watch. + if ok && mask&unix.IN_MOVE_SELF == unix.IN_MOVE_SELF { + err := w.remove(name, w.watches[name]) + if err != nil { + if !w.sendError(err) { + return + } + } + } w.mu.Unlock() if nameLen > 0 { diff --git a/backend_kqueue.go b/backend_kqueue.go index edf9aace..02ea2562 100644 --- a/backend_kqueue.go +++ b/backend_kqueue.go @@ -241,11 +241,11 @@ func (w *Watcher) Close() error { // // A path can only be watched once; attempting to watch it more than once will // return an error. Paths that do not yet exist on the filesystem cannot be -// added. A watch will be automatically removed if the path is deleted. +// added. // -// A path will remain watched if it gets renamed to somewhere else on the same -// filesystem, but the monitor will get removed if the path gets deleted and -// re-created, or if it's moved to a different filesystem. +// A watch will be automatically removed if the watched path is deleted or +// renamed. The exception is the Windows backend, which doesn't remove the +// watcher on renames. // // Notifications on network filesystems (NFS, SMB, FUSE, etc.) or special // filesystems (/proc, /sys, etc.) generally don't work. @@ -677,8 +677,8 @@ func (w *Watcher) sendFileCreatedEventIfNew(filePath string, fileInfo os.FileInf func (w *Watcher) internalWatch(name string, fileInfo os.FileInfo) (string, error) { if fileInfo.IsDir() { - // mimic Linux providing delete events for subdirectories - // but preserve the flags used if currently watching subdirectory + // mimic Linux providing delete events for subdirectories, but preserve + // the flags used if currently watching subdirectory w.mu.Lock() flags := w.dirFlags[name] w.mu.Unlock() diff --git a/backend_other.go b/backend_other.go index 9e2d082a..ae409b51 100644 --- a/backend_other.go +++ b/backend_other.go @@ -25,11 +25,11 @@ func (w *Watcher) Close() error { // // A path can only be watched once; attempting to watch it more than once will // return an error. Paths that do not yet exist on the filesystem cannot be -// added. A watch will be automatically removed if the path is deleted. +// added. // -// A path will remain watched if it gets renamed to somewhere else on the same -// filesystem, but the monitor will get removed if the path gets deleted and -// re-created, or if it's moved to a different filesystem. +// A watch will be automatically removed if the watched path is deleted or +// renamed. The exception is the Windows backend, which doesn't remove the +// watcher on renames. // // Notifications on network filesystems (NFS, SMB, FUSE, etc.) or special // filesystems (/proc, /sys, etc.) generally don't work. diff --git a/backend_windows.go b/backend_windows.go index fe09be87..5e62b4b4 100644 --- a/backend_windows.go +++ b/backend_windows.go @@ -196,11 +196,11 @@ func (w *Watcher) Close() error { // // A path can only be watched once; attempting to watch it more than once will // return an error. Paths that do not yet exist on the filesystem cannot be -// added. A watch will be automatically removed if the path is deleted. +// added. // -// A path will remain watched if it gets renamed to somewhere else on the same -// filesystem, but the monitor will get removed if the path gets deleted and -// re-created, or if it's moved to a different filesystem. +// A watch will be automatically removed if the watched path is deleted or +// renamed. The exception is the Windows backend, which doesn't remove the +// watcher on renames. // // Notifications on network filesystems (NFS, SMB, FUSE, etc.) or special // filesystems (/proc, /sys, etc.) generally don't work. diff --git a/fsnotify_test.go b/fsnotify_test.go index 54e446dc..be65676c 100644 --- a/fsnotify_test.go +++ b/fsnotify_test.go @@ -444,8 +444,6 @@ func TestWatchRename(t *testing.T) { `}, {"rename watched directory", func(t *testing.T, w *Watcher, tmp string) { - addWatch(t, w, tmp) - dir := filepath.Join(tmp, "dir") mkdir(t, dir) addWatch(t, w, dir) @@ -453,29 +451,14 @@ func TestWatchRename(t *testing.T) { mv(t, dir, tmp, "dir-renamed") touch(t, tmp, "dir-renamed/file") }, ` - CREATE "/dir" # mkdir - RENAME "/dir" # mv - CREATE "/dir-renamed" - RENAME "/dir" - CREATE "/dir/file" # touch - - windows: - CREATE "/dir" # mkdir - RENAME "/dir" # mv - CREATE "/dir-renamed" - CREATE "/dir-renamed/file" # touch + rename /dir - # TODO: no results for the touch; this is probably a bug; windows - # was fixed in #370. kqueue: - CREATE "/dir" # mkdir - CREATE "/dir-renamed" # mv - REMOVE|RENAME "/dir" - fen: - CREATE "/dir" # mkdir - RENAME "/dir" # mv - CREATE "/dir-renamed" - WRITE "/dir-renamed" # touch + remove|rename /dir + + # TODO(v2): Windows should behave the same by default. See #518 + windows: + create /dir/file `}, {"rename watched file", func(t *testing.T, w *Watcher, tmp string) { @@ -488,19 +471,12 @@ func TestWatchRename(t *testing.T) { mv(t, file, rename) mv(t, rename, tmp, "rename-two") }, ` - # TODO: this should update the path. And even then, not clear what - # go renamed to what. - rename /file # mv file rename - rename /file # mv rename rename-two - - # TODO: seems to lose the watch? - kqueue, fen: - rename /file + rename /file - # It's actually more correct on Windows. + # TODO(v2): Windows should behave the same by default. See #518 windows: - rename /file - rename /rename-one + rename /file + rename /rename-one `}, {"re-add renamed file", func(t *testing.T, w *Watcher, tmp string) { @@ -517,19 +493,14 @@ func TestWatchRename(t *testing.T) { cat(t, "hello", file) }, ` rename /file # mv file rename - write /rename # cat hello >rename + # Watcher gets removed on rename, so no write for /rename write /file # cat hello >file - # TODO: wrong. - linux: - RENAME "/file" - WRITE "/file" - WRITE "" - - # TODO: wrong. - kqueue, fen: - RENAME "/file" - WRITE "/file" + # TODO(v2): Windows should behave the same by default. See #518 + windows: + rename /file + write /rename + write /file `}, } diff --git a/mkdoc.zsh b/mkdoc.zsh index 664ec549..a0659b0c 100755 --- a/mkdoc.zsh +++ b/mkdoc.zsh @@ -78,11 +78,11 @@ add=$(<