From 42e2369070350d27f6da41c9e07d5f4cdbd40d28 Mon Sep 17 00:00:00 2001 From: Martin Tournoij Date: Thu, 13 Oct 2022 04:07:45 +0200 Subject: [PATCH] Return ErrClosed on Add() when the watcher is closed Also consistently return nil for Remove() and WatchList(). For Remove() we could return an error, but I don't really see the point: if the watcher is closed it's basically a no-op so returning nil is fine. For WatchList() I'd like to return an error, but there is no error return, so explicitly returning nil is the best we can do. Fixes #511 --- backend_fen.go | 18 +++++++++++++----- backend_inotify.go | 14 +++++++++++++- backend_kqueue.go | 13 ++++++++++++- backend_other.go | 2 ++ backend_windows.go | 16 +++++++++++++++- fsnotify.go | 5 +++-- fsnotify_test.go | 22 ++++++++++++++++++++++ mkdoc.zsh | 4 ++++ 8 files changed, 84 insertions(+), 10 deletions(-) diff --git a/backend_fen.go b/backend_fen.go index 0d87631d..721c67e0 100644 --- a/backend_fen.go +++ b/backend_fen.go @@ -196,6 +196,8 @@ func (w *Watcher) Close() error { // Notifications on network filesystems (NFS, SMB, FUSE, etc.) or special // filesystems (/proc, /sys, etc.) generally don't work. // +// Returns [ErrClosed] if [Watcher.Close] was called. +// // # Watching directories // // All files in a directory are monitored, including new files that are created @@ -215,7 +217,7 @@ func (w *Watcher) Close() error { // you're not interested in. There is an example of this in [cmd/fsnotify/file.go]. func (w *Watcher) Add(name string) error { if w.isClosed() { - return errors.New("FEN watcher already closed") + return ErrClosed } if w.port.PathIsWatched(name) { return nil @@ -260,7 +262,7 @@ func (w *Watcher) Add(name string) error { // Removing a path that has not yet been added returns [ErrNonExistentWatch]. func (w *Watcher) Remove(name string) error { if w.isClosed() { - return errors.New("FEN watcher already closed") + return nil } if !w.port.PathIsWatched(name) { return fmt.Errorf("%w: %s", ErrNonExistentWatch, name) @@ -528,7 +530,7 @@ func (w *Watcher) updateDirectory(path string) error { func (w *Watcher) associateFile(path string, stat os.FileInfo, follow bool) error { if w.isClosed() { - return errors.New("FEN watcher already closed") + return ErrClosed } // This is primarily protecting the call to AssociatePath // but it is important and intentional that the call to @@ -552,10 +554,10 @@ func (w *Watcher) associateFile(path string, stat os.FileInfo, follow bool) erro } } // FILE_NOFOLLOW means we watch symlinks themselves rather than their targets - events := unix.FILE_MODIFIED|unix.FILE_ATTRIB|unix.FILE_NOFOLLOW + events := unix.FILE_MODIFIED | unix.FILE_ATTRIB | unix.FILE_NOFOLLOW if follow { // We *DO* follow symlinks for explicitly watched entries - events = unix.FILE_MODIFIED|unix.FILE_ATTRIB + events = unix.FILE_MODIFIED | unix.FILE_ATTRIB } return w.port.AssociatePath(path, stat, events, @@ -570,7 +572,13 @@ func (w *Watcher) dissociateFile(path string, stat os.FileInfo, unused bool) err } // WatchList returns all paths added with [Add] (and are not yet removed). +// +// Returns nil if [Watcher.Close] was called. func (w *Watcher) WatchList() []string { + if w.isClosed() { + return nil + } + w.mu.Lock() defer w.mu.Unlock() diff --git a/backend_inotify.go b/backend_inotify.go index 54c77fbb..75736ce7 100644 --- a/backend_inotify.go +++ b/backend_inotify.go @@ -218,6 +218,8 @@ func (w *Watcher) Close() error { // Notifications on network filesystems (NFS, SMB, FUSE, etc.) or special // filesystems (/proc, /sys, etc.) generally don't work. // +// Returns [ErrClosed] if [Watcher.Close] was called. +// // # Watching directories // // All files in a directory are monitored, including new files that are created @@ -238,7 +240,7 @@ func (w *Watcher) Close() error { func (w *Watcher) Add(name string) error { name = filepath.Clean(name) if w.isClosed() { - return errors.New("inotify instance already closed") + return ErrClosed } var flags uint32 = unix.IN_MOVED_TO | unix.IN_MOVED_FROM | @@ -274,6 +276,10 @@ func (w *Watcher) Add(name string) error { // // Removing a path that has not yet been added returns [ErrNonExistentWatch]. func (w *Watcher) Remove(name string) error { + if w.isClosed() { + return nil + } + name = filepath.Clean(name) // Fetch the watch. @@ -317,7 +323,13 @@ func (w *Watcher) Remove(name string) error { } // WatchList returns all paths added with [Add] (and are not yet removed). +// +// Returns nil if [Watcher.Close] was called. func (w *Watcher) WatchList() []string { + if w.isClosed() { + return nil + } + w.mu.Lock() defer w.mu.Unlock() diff --git a/backend_kqueue.go b/backend_kqueue.go index 29087469..edf9aace 100644 --- a/backend_kqueue.go +++ b/backend_kqueue.go @@ -250,6 +250,8 @@ func (w *Watcher) Close() error { // Notifications on network filesystems (NFS, SMB, FUSE, etc.) or special // filesystems (/proc, /sys, etc.) generally don't work. // +// Returns [ErrClosed] if [Watcher.Close] was called. +// // # Watching directories // // All files in a directory are monitored, including new files that are created @@ -284,6 +286,10 @@ func (w *Watcher) Add(name string) error { func (w *Watcher) Remove(name string) error { name = filepath.Clean(name) w.mu.Lock() + if w.isClosed { + w.mu.Unlock() + return nil + } watchfd, ok := w.watches[name] w.mu.Unlock() if !ok { @@ -337,9 +343,14 @@ func (w *Watcher) Remove(name string) error { } // WatchList returns all paths added with [Add] (and are not yet removed). +// +// Returns nil if [Watcher.Close] was called. func (w *Watcher) WatchList() []string { w.mu.Lock() defer w.mu.Unlock() + if w.isClosed { + return nil + } entries := make([]string, 0, len(w.userWatches)) for pathname := range w.userWatches { @@ -363,7 +374,7 @@ func (w *Watcher) addWatch(name string, flags uint32) (string, error) { w.mu.Lock() if w.isClosed { w.mu.Unlock() - return "", errors.New("kevent instance already closed") + return "", ErrClosed } watchfd, alreadyWatching := w.watches[name] // We already have a watch, but we can still override flags. diff --git a/backend_other.go b/backend_other.go index a9bb1c3c..9e2d082a 100644 --- a/backend_other.go +++ b/backend_other.go @@ -34,6 +34,8 @@ func (w *Watcher) Close() error { // Notifications on network filesystems (NFS, SMB, FUSE, etc.) or special // filesystems (/proc, /sys, etc.) generally don't work. // +// Returns [ErrClosed] if [Watcher.Close] was called. +// // # Watching directories // // All files in a directory are monitored, including new files that are created diff --git a/backend_windows.go b/backend_windows.go index ae392867..91c78c8c 100644 --- a/backend_windows.go +++ b/backend_windows.go @@ -199,6 +199,8 @@ func (w *Watcher) Close() error { // Notifications on network filesystems (NFS, SMB, FUSE, etc.) or special // filesystems (/proc, /sys, etc.) generally don't work. // +// Returns [ErrClosed] if [Watcher.Close] was called. +// // # Watching directories // // All files in a directory are monitored, including new files that are created @@ -220,7 +222,7 @@ func (w *Watcher) Add(name string) error { w.mu.Lock() if w.isClosed { w.mu.Unlock() - return errors.New("watcher already closed") + return ErrClosed } w.mu.Unlock() @@ -244,6 +246,13 @@ func (w *Watcher) Add(name string) error { // // Removing a path that has not yet been added returns [ErrNonExistentWatch]. func (w *Watcher) Remove(name string) error { + w.mu.Lock() + if w.isClosed { + w.mu.Unlock() + return nil + } + w.mu.Unlock() + in := &input{ op: opRemoveWatch, path: filepath.Clean(name), @@ -257,9 +266,14 @@ func (w *Watcher) Remove(name string) error { } // WatchList returns all paths added with [Add] (and are not yet removed). +// +// Returns nil if [Watcher.Close] was called. func (w *Watcher) WatchList() []string { w.mu.Lock() defer w.mu.Unlock() + if w.isClosed { + return nil + } entries := make([]string, 0, len(w.watches)) for _, entry := range w.watches { diff --git a/fsnotify.go b/fsnotify.go index 30a5bf0f..555a02bb 100644 --- a/fsnotify.go +++ b/fsnotify.go @@ -42,8 +42,9 @@ const ( // Common errors that can be reported by a watcher var ( - ErrNonExistentWatch = errors.New("can't remove non-existent watcher") - ErrEventOverflow = errors.New("fsnotify queue overflow") + ErrNonExistentWatch = errors.New("fsnotify: can't remove non-existent watcher") + ErrEventOverflow = errors.New("fsnotify: queue overflow") + ErrClosed = errors.New("fsnotify: watcher already closed") ) func (op Op) String() string { diff --git a/fsnotify_test.go b/fsnotify_test.go index ad322d35..d48ee2b0 100644 --- a/fsnotify_test.go +++ b/fsnotify_test.go @@ -892,6 +892,28 @@ func TestClose(t *testing.T) { chanClosed(t, w.w) }) + + t.Run("error after closed", func(t *testing.T) { + t.Parallel() + + tmp := t.TempDir() + w := newWatcher(t, tmp) + if err := w.Close(); err != nil { + t.Fatal(err) + } + + file := filepath.Join(tmp, "file") + touch(t, file) + if err := w.Add(file); !errors.Is(err, ErrClosed) { + t.Fatalf("wrong error for Add: %#v", err) + } + if err := w.Remove(file); err != nil { + t.Fatalf("wrong error for Remove: %#v", err) + } + if ugh := w.WatchList(); ugh != nil { // This function is crap. + t.Fatalf("WatchList not nil: %#v", ugh) + } + }) } func TestAdd(t *testing.T) { diff --git a/mkdoc.zsh b/mkdoc.zsh index b09ef768..664ec549 100755 --- a/mkdoc.zsh +++ b/mkdoc.zsh @@ -87,6 +87,8 @@ add=$(<