Return ErrClosed on Add() when the watcher is closed (#516)

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

CHANGELOG.md

@@ -9,9 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Additions
 
-- Add FEN backend to support illumos and Solaris. ([#371])
+- illumos: add FEN backend to support illumos and Solaris. ([#371])
+
+### Changes and fixes
+
+- 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
 
 ## [1.6.0] - 2022-10-13
 

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
@@ -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()
 

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()
 

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.

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

backend_windows.go

@@ -120,9 +120,9 @@ type Watcher struct {
 	input chan *input    // Inputs to the reader are sent on this channel
 	quit  chan chan<- error
 
-	mu       sync.Mutex // Protects access to watches, isClosed
-	watches  watchMap   // Map of watches (key: i-number)
-	isClosed bool       // Set to true when Close() is first called
+	mu      sync.Mutex // Protects access to watches, closed
+	watches watchMap   // Map of watches (key: i-number)
+	closed  bool       // Set to true when Close() is first called
 }
 
 // NewWatcher creates a new Watcher.
@@ -143,6 +143,12 @@ func NewWatcher() (*Watcher, error) {
 	return w, nil
 }
 
+func (w *Watcher) isClosed() bool {
+	w.mu.Lock()
+	defer w.mu.Unlock()
+	return w.closed
+}
+
 func (w *Watcher) sendEvent(name string, mask uint64) bool {
 	if mask == 0 {
 		return false
@@ -169,12 +175,12 @@ func (w *Watcher) sendError(err error) bool {
 
 // Close removes all watches and closes the events channel.
 func (w *Watcher) Close() error {
-	w.mu.Lock()
-	if w.isClosed {
-		w.mu.Unlock()
+	if w.isClosed() {
 		return nil
 	}
-	w.isClosed = true
+
+	w.mu.Lock()
+	w.closed = true
 	w.mu.Unlock()
 
 	// Send "quit" message to the reader goroutine
@@ -199,6 +205,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
@@ -217,12 +225,9 @@ func (w *Watcher) Close() error {
 // Instead, watch the parent directory and use Event.Name to filter out files
 // you're not interested in. There is an example of this in [cmd/fsnotify/file.go].
 func (w *Watcher) Add(name string) error {
-	w.mu.Lock()
-	if w.isClosed {
-		w.mu.Unlock()
-		return errors.New("watcher already closed")
+	if w.isClosed() {
+		return ErrClosed
 	}
-	w.mu.Unlock()
 
 	in := &input{
 		op:    opAddWatch,
@@ -244,6 +249,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
+	}
+
 	in := &input{
 		op:    opRemoveWatch,
 		path:  filepath.Clean(name),
@@ -257,7 +266,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()
 

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 (o Op) String() string {

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 l := w.WatchList(); l != nil { // Should return an error, but meh :-/
+			t.Fatalf("WatchList not nil: %#v", l)
+		}
+	})
 }
 
 func TestAdd(t *testing.T) {

mkdoc.zsh

@@ -87,6 +87,8 @@ add=$(<<EOF
 // 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
@@ -124,6 +126,8 @@ EOF
 
 watchlist=$(<<EOF
 // WatchList returns all paths added with [Add] (and are not yet removed).
+//
+// Returns nil if [Watcher.Close] was called.
 EOF
 )