From: Pieter Droogendijk Date: Fri, 6 Feb 2015 01:47:13 +0000 (+0100) Subject: inotify: closing watcher should now always shut down goroutine X-Git-Tag: v1.7.2~238 X-Git-Url: https://go.fuhry.dev/?a=commitdiff_plain;h=82a2c3da516da93b9e300d687e8bdb6c5ffe0aa8;p=fsnotify.git inotify: closing watcher should now always shut down goroutine (fixes #3) closes #63 --- diff --git a/AUTHORS b/AUTHORS index 440cda4..4e0e828 100644 --- a/AUTHORS +++ b/AUTHORS @@ -21,6 +21,7 @@ Kelvin Fo Matt Layher Nathan Youngman Paul Hammond +Pieter Droogendijk Pursuit92 Rob Figueiredo Soge Zhang diff --git a/CHANGELOG.md b/CHANGELOG.md index fe4c3ba..bfbe734 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## master / 2015-02-07 + +* inotify: closing watcher should now always shut down goroutine [#63](https://github.com/go-fsnotify/fsnotify/pull/63) (thanks @PieterD) + ## v1.1.1 / 2015-02-05 * inotify: Retry read on EINTR [#61](https://github.com/go-fsnotify/fsnotify/issues/61) (thanks @PieterD) diff --git a/inotify.go b/inotify.go index 1a693f3..b61ae12 100644 --- a/inotify.go +++ b/inotify.go @@ -19,14 +19,13 @@ import ( // Watcher watches a set of files, delivering events to a channel. type Watcher struct { - Events chan Event - Errors chan error - mu sync.Mutex // Map access - fd int // File descriptor (as returned by the inotify_init() syscall) - watches map[string]*watch // Map of inotify watches (key: path) - paths map[int]string // Map of watched paths (key: watch descriptor) - done chan bool // Channel for sending a "quit message" to the reader goroutine - isClosed bool // Set to true when Close() is first called + Events chan Event + Errors chan error + mu sync.Mutex // Map access + fd int // File descriptor (as returned by the inotify_init() syscall) + watches map[string]*watch // Map of inotify watches (key: path) + paths map[int]string // Map of watched paths (key: watch descriptor) + done chan struct{} // Channel for sending a "quit message" to the reader goroutine } // NewWatcher establishes a new watcher with the underlying OS and begins waiting for events. @@ -41,27 +40,44 @@ func NewWatcher() (*Watcher, error) { paths: make(map[int]string), Events: make(chan Event), Errors: make(chan error), - done: make(chan bool, 1), + done: make(chan struct{}), } go w.readEvents() return w, nil } +func (w *Watcher) isClosed() bool { + select { + case <-w.done: + return true + default: + return false + } +} + // Close removes all watches and closes the events channel. func (w *Watcher) Close() error { - if w.isClosed { + if w.isClosed() { return nil } - w.isClosed = true - // Remove all watches - for name := range w.watches { - w.Remove(name) - } + // Send 'close' signal to goroutine, and set the Watcher to closed. + close(w.done) - // Send "quit" message to the reader goroutine - w.done <- true + // Remove all watches. + // Everything after this may generate errors because the inotify channel + // has been closed; we don't care. + numWatches := w.removeAll() + + // If no watches were removed, it's possible syscall.Read is still blocking. + // In this case, create a watch and remove it to wake it up. + // If that fails, there's really nothing left to do. we've done our best, + // but the goroutine may be alive forever. + if numWatches == 0 { + wd, _ := syscall.InotifyAddWatch(w.fd, ".", syscall.IN_DELETE_SELF) + syscall.InotifyRmWatch(w.fd, uint32(wd)) + } return nil } @@ -69,7 +85,7 @@ func (w *Watcher) Close() error { // Add starts watching the named file or directory (non-recursively). func (w *Watcher) Add(name string) error { name = filepath.Clean(name) - if w.isClosed { + if w.isClosed() { return errors.New("inotify instance already closed") } @@ -99,12 +115,16 @@ func (w *Watcher) Add(name string) error { return nil } -// Remove stops watching the the named file or directory (non-recursively). +// Remove stops watching the named file or directory (non-recursively). func (w *Watcher) Remove(name string) error { name = filepath.Clean(name) + + // Fetch the watch. w.mu.Lock() defer w.mu.Unlock() watch, ok := w.watches[name] + + // Remove it from inotify. if !ok { return fmt.Errorf("can't remove non-existent inotify watch for: %s", name) } @@ -116,6 +136,21 @@ func (w *Watcher) Remove(name string) error { return nil } +// removeAll watches +func (w *Watcher) removeAll() int { + removed := 0 + w.mu.Lock() + defer w.mu.Unlock() + for name, watch := range w.watches { + success, _ := syscall.InotifyRmWatch(w.fd, watch.wd) + if success != -1 { + removed++ + } + delete(w.watches, name) + } + return removed +} + type watch struct { wd uint32 // Watch descriptor (as returned by the inotify_add_watch() syscall) flags uint32 // inotify flags of this watch (see inotify(7) for the list of valid flags) @@ -130,15 +165,14 @@ func (w *Watcher) readEvents() { errno error // Syscall errno ) + defer close(w.Errors) + defer close(w.Events) + defer syscall.Close(w.fd) + for { - // See if there is a message on the "done" channel - select { - case <-w.done: - syscall.Close(w.fd) - close(w.Events) - close(w.Errors) + // See if we have been closed. + if w.isClosed() { return - default: } n, errno = syscall.Read(w.fd, buf[:]) @@ -149,20 +183,31 @@ func (w *Watcher) readEvents() { continue } + // syscall.Read might have been woken up by Close. If so, we're done. + if w.isClosed() { + return + } + // If EOF is received if n == 0 { - syscall.Close(w.fd) - close(w.Events) - close(w.Errors) + close(w.done) return } if n < 0 { - w.Errors <- os.NewSyscallError("read", errno) + select { + case w.Errors <- os.NewSyscallError("read", errno): + case <-w.done: + return + } continue } if n < syscall.SizeofInotifyEvent { - w.Errors <- errors.New("inotify: short read in readEvents()") + select { + case w.Errors <- errors.New("inotify: short read in readEvents()"): + case <-w.done: + return + } continue } @@ -193,7 +238,11 @@ func (w *Watcher) readEvents() { // Send the events that are not ignored on the events channel if !event.ignoreLinux(mask) { - w.Events <- event + select { + case w.Events <- event: + case <-w.done: + return + } } // Move to the next event in the buffer diff --git a/inotify_test.go b/inotify_test.go new file mode 100644 index 0000000..21aa924 --- /dev/null +++ b/inotify_test.go @@ -0,0 +1,108 @@ +// Copyright 2015 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// +build linux + +package fsnotify + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +func TestInotifyCloseRightAway(t *testing.T) { + w, err := NewWatcher() + if err != nil { + t.Fatalf("Failed to create watcher") + } + + // Close immediately; it won't even reach the first syscall.Read. + w.Close() + + // Wait for the close to complete. + <-time.After(50 * time.Millisecond) + isWatcherReallyClosed(t, w) +} + +func TestInotifyCloseSlightlyLater(t *testing.T) { + w, err := NewWatcher() + if err != nil { + t.Fatalf("Failed to create watcher") + } + + // Wait until readEvents has reached syscall.Read, and Close. + <-time.After(50 * time.Millisecond) + w.Close() + + // Wait for the close to complete. + <-time.After(50 * time.Millisecond) + isWatcherReallyClosed(t, w) +} + +func TestInotifyCloseSlightlyLaterWithWatch(t *testing.T) { + testDir := tempMkdir(t) + defer os.RemoveAll(testDir) + + w, err := NewWatcher() + if err != nil { + t.Fatalf("Failed to create watcher") + } + w.Add(testDir) + + // Wait until readEvents has reached syscall.Read, and Close. + <-time.After(50 * time.Millisecond) + w.Close() + + // Wait for the close to complete. + <-time.After(50 * time.Millisecond) + isWatcherReallyClosed(t, w) +} + +func TestInotifyCloseAfterRead(t *testing.T) { + testDir := tempMkdir(t) + defer os.RemoveAll(testDir) + + w, err := NewWatcher() + if err != nil { + t.Fatalf("Failed to create watcher") + } + + err = w.Add(testDir) + if err != nil { + t.Fatalf("Failed to add .") + } + + // Generate an event. + os.Create(filepath.Join(testDir, "somethingSOMETHINGsomethingSOMETHING")) + + // Wait for readEvents to read the event, then close the watcher. + <-time.After(50 * time.Millisecond) + w.Close() + + // Wait for the close to complete. + <-time.After(50 * time.Millisecond) + isWatcherReallyClosed(t, w) +} + +func isWatcherReallyClosed(t *testing.T, w *Watcher) { + select { + case err, ok := <-w.Errors: + if ok { + t.Fatalf("w.Errors is not closed; readEvents is still alive after closing (error: %v)", err) + } + default: + t.Fatalf("w.Errors would have blocked; readEvents is still alive!") + } + + select { + case _, ok := <-w.Events: + if ok { + t.Fatalf("w.Events is not closed; readEvents is still alive after closing") + } + default: + t.Fatalf("w.Events would have blocked; readEvents is still alive!") + } +}