inotify: fix race in Close() (#465)

Would sometimes fail with:

	panic: close of closed channel

	goroutine 204 [running]:
	github.com/fsnotify/fsnotify.(*Watcher).Close(0xc0003e6410)
        	/home/martin/code/Golib/fsnotify/inotify.go:82 +0x66
	created by github.com/fsnotify/fsnotify.TestCloseRace
        	/home/martin/code/Golib/fsnotify/integration_test.go:1256 +0x1a5
	exit status 2

Because isClosed() might return "false" for two goroutines, and then
they will both try to close it, resulting in the panic.

Fixes #367

diff --git a/inotify.go b/inotify.go
index 9e020ed4be2259e03464b6f4484b65c767d670bb..b01124a63203371eb7ad5356b966bc8a880830a6 100644 (file)
--- a/inotify.go
+++ b/inotify.go
@@ -72,12 +72,15 @@ func (w *Watcher) isClosed() bool {
 
 // Close removes all watches and closes the events channel.
 func (w *Watcher) Close() error {
+       w.mu.Lock()
        if w.isClosed() {
+               w.mu.Unlock()
                return nil
        }
 
        // Send 'close' signal to goroutine, and set the Watcher to closed.
        close(w.done)
+       w.mu.Unlock()
 
        // Wake up goroutine
        w.poller.wake()

diff --git a/inotify_test.go b/inotify_test.go
index 361f19823b59c87f4c1ec927cb8b89a7f7599852..269c3ff80a24ea17aad006c24bca1dd0038402a8 100644 (file)
--- a/inotify_test.go
+++ b/inotify_test.go
@@ -354,14 +354,16 @@ func TestInotifyInnerMapLength(t *testing.T) {
        _ = <-w.Events                      // consume Remove event
        <-time.After(50 * time.Millisecond) // wait IN_IGNORE propagated
 
-       w.mu.Lock()
-       defer w.mu.Unlock()
-       if len(w.watches) != 0 {
-               t.Fatalf("Expected watches len is 0, but got: %d, %v", len(w.watches), w.watches)
-       }
-       if len(w.paths) != 0 {
-               t.Fatalf("Expected paths len is 0, but got: %d, %v", len(w.paths), w.paths)
-       }
+       func() {
+               w.mu.Lock()
+               defer w.mu.Unlock()
+               if len(w.watches) != 0 {
+                       t.Fatalf("Expected watches len is 0, but got: %d, %v", len(w.watches), w.watches)
+               }
+               if len(w.paths) != 0 {
+                       t.Fatalf("Expected paths len is 0, but got: %d, %v", len(w.paths), w.paths)
+               }
+       }()
 
        w.Close()
        wg.Wait()

diff --git a/integration_test.go b/integration_test.go
index 9dace27b1aa949ce931a99d5dc250ac3997b9ccd..fefae5ea3b7529cd4f5c7d059a40538bd9183d43 100644 (file)
--- a/integration_test.go
+++ b/integration_test.go
@@ -1244,6 +1244,21 @@ func TestRemoveWithClose(t *testing.T) {
        }
 }
 
+// Make sure Close() doesn't race; hard to write a good reproducible test for
+// this, but running it 150 times seems to reproduce it in ~75% of cases and
+// isn't too slow (~0.06s on my system).
+func TestCloseRace(t *testing.T) {
+       for i := 0; i < 150; i++ {
+               w, err := NewWatcher()
+               if err != nil {
+                       t.Fatal(err)
+               }
+               go w.Close()
+               go w.Close()
+               go w.Close()
+       }
+}
+
 func testRename(file1, file2 string) error {
        switch runtime.GOOS {
        case "windows", "plan9":