From 8611c35ab31c1c28aa903d33cf8b6e44a399b09e Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bj=C3=B8rn=20Erik=20Pedersen?= Date: Sun, 10 Jan 2016 16:04:37 +0100 Subject: [PATCH] kqueue: fix Remove-logic `os.Lstat` does not return `error` when a file/directory exists, so the code path to check for newly created files after a REMOVE on BSD was never executed. Also, if the watched entry is a file, it has been removed from the watcher, so do a new-file-check for them as well. This commit fixes issues on TextMate on OS X and others that uses `exchangedata` to do atomic saves. See #17. closes #111. --- CHANGELOG.md | 4 + integration_darwin_test.go | 146 +++++++++++++++++++++++++++++++++++++ kqueue.go | 66 ++++++++++------- 3 files changed, 191 insertions(+), 25 deletions(-) create mode 100644 integration_darwin_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index da6aa28..d37247e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## v1.2.9 / 2016-01-13 + +kqueue: Fix logic for CREATE after REMOVE [#111](https://github.com/go-fsnotify/fsnotify/pull/111) (thanks @bep) + ## v1.2.8 / 2015-12-17 * kqueue: fix race condition in Close [#105](https://github.com/go-fsnotify/fsnotify/pull/105) (thanks @djui for reporting the issue and @ppknap for writing a failing test) diff --git a/integration_darwin_test.go b/integration_darwin_test.go new file mode 100644 index 0000000..1b8982f --- /dev/null +++ b/integration_darwin_test.go @@ -0,0 +1,146 @@ +// Copyright 2016 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. + +package fsnotify + +import ( + "os" + "path/filepath" + "syscall" + "testing" + "time" +) + +// testExchangedataForWatcher tests the watcher with the exchangedata operation on OS X. +// +// This is widely used for atomic saves on OS X, e.g. TextMate and in Apple's NSDocument. +// +// See https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man2/exchangedata.2.html +// Also see: https://github.com/textmate/textmate/blob/cd016be29489eba5f3c09b7b70b06da134dda550/Frameworks/io/src/swap_file_data.cc#L20 +func testExchangedataForWatcher(t *testing.T, watchDir bool) { + // Create directory to watch + testDir1 := tempMkdir(t) + + // For the intermediate file + testDir2 := tempMkdir(t) + + defer os.RemoveAll(testDir1) + defer os.RemoveAll(testDir2) + + resolvedFilename := "TestFsnotifyEvents.file" + + // TextMate does: + // + // 1. exchangedata (intermediate, resolved) + // 2. unlink intermediate + // + // Let's try to simulate that: + resolved := filepath.Join(testDir1, resolvedFilename) + intermediate := filepath.Join(testDir2, resolvedFilename+"~") + + // Make sure we create the file before we start watching + createAndSyncFile(t, resolved) + + watcher := newWatcher(t) + + // Test both variants in isolation + if watchDir { + addWatch(t, watcher, testDir1) + } else { + addWatch(t, watcher, resolved) + } + + // Receive errors on the error channel on a separate goroutine + go func() { + for err := range watcher.Errors { + t.Fatalf("error received: %s", err) + } + }() + + // Receive events on the event channel on a separate goroutine + eventstream := watcher.Events + var removeReceived counter + var createReceived counter + + done := make(chan bool) + + go func() { + for event := range eventstream { + // Only count relevant events + if event.Name == filepath.Clean(resolved) { + if event.Op&Remove == Remove { + removeReceived.increment() + } + if event.Op&Create == Create { + createReceived.increment() + } + } + t.Logf("event received: %s", event) + } + done <- true + }() + + // Repeat to make sure the watched file/directory "survives" the REMOVE/CREATE loop. + for i := 1; i <= 3; i++ { + // The intermediate file is created in a folder outside the watcher + createAndSyncFile(t, intermediate) + + // 1. Swap + if err := syscall.Exchangedata(intermediate, resolved, 0); err != nil { + t.Fatalf("[%d] exchangedata failed: %s", i, err) + } + + time.Sleep(50 * time.Millisecond) + + // 2. Delete the intermediate file + err := os.Remove(intermediate) + + if err != nil { + t.Fatalf("[%d] remove %s failed: %s", i, intermediate, err) + } + + time.Sleep(50 * time.Millisecond) + + } + + // We expect this event to be received almost immediately, but let's wait 500 ms to be sure + time.Sleep(500 * time.Millisecond) + + // The events will be (CHMOD + REMOVE + CREATE) X 2. Let's focus on the last two: + if removeReceived.value() < 3 { + t.Fatal("fsnotify remove events have not been received after 500 ms") + } + + if createReceived.value() < 3 { + t.Fatal("fsnotify create events have not been received after 500 ms") + } + + watcher.Close() + t.Log("waiting for the event channel to become closed...") + select { + case <-done: + t.Log("event channel closed") + case <-time.After(2 * time.Second): + t.Fatal("event stream was not closed after 2 seconds") + } +} + +// TestExchangedataInWatchedDir test exchangedata operation on file in watched dir. +func TestExchangedataInWatchedDir(t *testing.T) { + testExchangedataForWatcher(t, true) +} + +// TestExchangedataInWatchedDir test exchangedata operation on watched file. +func TestExchangedataInWatchedFile(t *testing.T) { + testExchangedataForWatcher(t, false) +} + +func createAndSyncFile(t *testing.T, filepath string) { + f1, err := os.OpenFile(filepath, os.O_WRONLY|os.O_CREATE, 0666) + if err != nil { + t.Fatalf("creating %s failed: %s", filepath, err) + } + f1.Sync() + f1.Close() +} diff --git a/kqueue.go b/kqueue.go index 7a7700b..b8ea308 100644 --- a/kqueue.go +++ b/kqueue.go @@ -325,19 +325,24 @@ func (w *Watcher) readEvents() { if event.Op&Remove == Remove { // Look for a file that may have overwritten this. // For example, mv f1 f2 will delete f2, then create f2. - fileDir, _ := filepath.Split(event.Name) - fileDir = filepath.Clean(fileDir) - w.mu.Lock() - _, found := w.watches[fileDir] - w.mu.Unlock() - if found { - // make sure the directory exists before we watch for changes. When we - // do a recursive watch and perform rm -fr, the parent directory might - // have gone missing, ignore the missing directory and let the - // upcoming delete event remove the watch from the parent directory. - if _, err := os.Lstat(fileDir); os.IsExist(err) { - w.sendDirectoryChangeEvents(fileDir) - // FIXME: should this be for events on files or just isDir? + if path.isDir { + fileDir := filepath.Clean(event.Name) + w.mu.Lock() + _, found := w.watches[fileDir] + w.mu.Unlock() + if found { + // make sure the directory exists before we watch for changes. When we + // do a recursive watch and perform rm -fr, the parent directory might + // have gone missing, ignore the missing directory and let the + // upcoming delete event remove the watch from the parent directory. + if _, err := os.Lstat(fileDir); err == nil { + w.sendDirectoryChangeEvents(fileDir) + } + } + } else { + filePath := filepath.Clean(event.Name) + if fileInfo, err := os.Lstat(filePath); err == nil { + w.sendFileCreatedEventIfNew(filePath, fileInfo) } } } @@ -407,24 +412,35 @@ func (w *Watcher) sendDirectoryChangeEvents(dirPath string) { // Search for new files for _, fileInfo := range files { filePath := filepath.Join(dirPath, fileInfo.Name()) - w.mu.Lock() - _, doesExist := w.fileExists[filePath] - w.mu.Unlock() - if !doesExist { - // Send create event - w.Events <- newCreateEvent(filePath) - } + err := w.sendFileCreatedEventIfNew(filePath, fileInfo) - // like watchDirectoryFiles (but without doing another ReadDir) - filePath, err = w.internalWatch(filePath, fileInfo) if err != nil { return } + } +} - w.mu.Lock() - w.fileExists[filePath] = true - w.mu.Unlock() +// sendFileCreatedEvent sends a create event if the file isn't already being tracked. +func (w *Watcher) sendFileCreatedEventIfNew(filePath string, fileInfo os.FileInfo) (err error) { + w.mu.Lock() + _, doesExist := w.fileExists[filePath] + w.mu.Unlock() + if !doesExist { + // Send create event + w.Events <- newCreateEvent(filePath) + } + + // like watchDirectoryFiles (but without doing another ReadDir) + filePath, err = w.internalWatch(filePath, fileInfo) + if err != nil { + return err } + + w.mu.Lock() + w.fileExists[filePath] = true + w.mu.Unlock() + + return nil } func (w *Watcher) internalWatch(name string, fileInfo os.FileInfo) (string, error) { -- 2.50.1