kqueue: fix race condition in Close

fixes #99
closes #105

thanks to @djui for reporting the issue and @ppknap for writing a test.

Add test for concurrent file removes and watcher.Close calls

diff --git a/integration_test.go b/integration_test.go
index 49e48ff1ca7da214c06b001cbac5ecf426825f83..8b7e9d3ec8144b177369155bd3951d8fe705e881 100644 (file)
--- a/integration_test.go
+++ b/integration_test.go
@@ -44,6 +44,16 @@ func tempMkdir(t *testing.T) string {
        return dir
 }
 
+// tempMkFile makes a temporary file.
+func tempMkFile(t *testing.T, dir string) string {
+       f, err := ioutil.TempFile(dir, "fsnotify")
+       if err != nil {
+               t.Fatalf("failed to create test file: %v", err)
+       }
+       defer f.Close()
+       return f.Name()
+}
+
 // newWatcher initializes an fsnotify Watcher instance.
 func newWatcher(t *testing.T) *Watcher {
        watcher, err := NewWatcher()
@@ -1172,6 +1182,50 @@ func TestClose(t *testing.T) {
        }
 }
 
+// TestRemoveWithClose tests if one can handle Remove events and, at the same
+// time, close Watcher object without any data races.
+func TestRemoveWithClose(t *testing.T) {
+       testDir := tempMkdir(t)
+       defer os.RemoveAll(testDir)
+
+       const fileN = 200
+       tempFiles := make([]string, 0, fileN)
+       for i := 0; i < fileN; i++ {
+               tempFiles = append(tempFiles, tempMkFile(t, testDir))
+       }
+       watcher := newWatcher(t)
+       if err := watcher.Add(testDir); err != nil {
+               t.Fatalf("Expected no error on Add, got %v", err)
+       }
+       startC, stopC := make(chan struct{}), make(chan struct{})
+       errC := make(chan error)
+       go func() {
+               for {
+                       select {
+                       case <-watcher.Errors:
+                       case <-watcher.Events:
+                       case <-stopC:
+                               return
+                       }
+               }
+       }()
+       go func() {
+               <-startC
+               for _, fileName := range tempFiles {
+                       os.Remove(fileName)
+               }
+       }()
+       go func() {
+               <-startC
+               errC <- watcher.Close()
+       }()
+       close(startC)
+       defer close(stopC)
+       if err := <-errC; err != nil {
+               t.Fatalf("Expected no error on Close, got %v.", err)
+       }
+}
+
 func testRename(file1, file2 string) error {
        switch runtime.GOOS {
        case "windows", "plan9":

diff --git a/kqueue.go b/kqueue.go
index 9662a50a84d2aa87ec361c85cedf99b7158872b5..7a7700bac7bb61173a1a2b14bc4e1f850de05986 100644 (file)
--- a/kqueue.go
+++ b/kqueue.go
@@ -72,12 +72,17 @@ func (w *Watcher) Close() error {
        w.isClosed = true
        w.mu.Unlock()
 
+       // copy paths to remove while locked
        w.mu.Lock()
-       ws := w.watches
+       var pathsToRemove = make([]string, 0, len(w.watches))
+       for name := range w.watches {
+               pathsToRemove = append(pathsToRemove, name)
+       }
        w.mu.Unlock()
+       // unlock before calling Remove, which also locks
 
        var err error
-       for name := range ws {
+       for _, name := range pathsToRemove {
                if e := w.Remove(name); e != nil && err == nil {
                        err = e
                }