diff --git a/CHANGELOG.md b/CHANGELOG.md index 362e81d8..74b32383 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,9 +39,17 @@ This version of fsnotify needs Go 1.17. Before it would merely return "short read", making it hard to detect this error. -- all: return `ErrClosed` on `Add()` when the watcher is closed ([#516]) +- kqueue: make sure events for all files are delivered properly when removing a + watched directory ([#526]) -- kqueue: deal with `rm -rf watched-dir` better ([#526], [#537]) + Previously they would get sent with "" or "." as the path name. + +- kqueue: don't emit spurious Create events for symbolic links ([#524]) + + The link would get resolved but kqueue would "forget" it already saw the link + itself, resulting on a Create for every Write event for the directory. + +- all: return ErrClosed on Add() when the watcher is closed ([#516]) - other: add `Watcher.Errors` and `Watcher.Events` to the no-op `Watcher` in `backend_other.go`, making it easier to use on unsupported platforms such as diff --git a/backend_kqueue.go b/backend_kqueue.go index 9d464504..3177561b 100644 --- a/backend_kqueue.go +++ b/backend_kqueue.go @@ -400,9 +400,10 @@ func (w *Watcher) WatchList() []string { // Watch all events (except NOTE_EXTEND, NOTE_LINK, NOTE_REVOKE) const noteAllEvents = unix.NOTE_DELETE | unix.NOTE_WRITE | unix.NOTE_ATTRIB | unix.NOTE_RENAME -// addWatch adds name to the watched file set. -// The flags are interpreted as described in kevent(2). -// Returns the real path to the file which was added, if any, which may be different from the one passed in the case of symlinks. +// addWatch adds name to the watched file set; the flags are interpreted as +// described in kevent(2). +// +// Returns the real path to the file which was added, with symlinks resolved. func (w *Watcher) addWatch(name string, flags uint32) (string, error) { var isDir bool name = filepath.Clean(name) @@ -430,27 +431,30 @@ func (w *Watcher) addWatch(name string, flags uint32) (string, error) { return "", nil } - // Follow Symlinks - // - // Linux can add unresolvable symlinks to the watch list without issue, - // and Windows can't do symlinks period. To maintain consistency, we - // will act like everything is fine if the link can't be resolved. - // There will simply be no file events for broken symlinks. Hence the - // returns of nil on errors. + // Follow Symlinks. if fi.Mode()&os.ModeSymlink == os.ModeSymlink { - name, err = filepath.EvalSymlinks(name) + link, err := os.Readlink(name) if err != nil { + // Return nil because Linux can add unresolvable symlinks to the + // watch list without problems, so maintain consistency with + // that. There will be no file events for broken symlinks. + // TODO: more specific check; returns os.PathError; ENOENT? return "", nil } w.mu.Lock() - _, alreadyWatching = w.watches[name] + _, alreadyWatching = w.watches[link] w.mu.Unlock() if alreadyWatching { - return name, nil + // Add to watches so we don't get spurious Create events later + // on when we diff the directories. + w.watches[name] = 0 + w.fileExists[name] = struct{}{} + return link, nil } + name = link fi, err = os.Lstat(name) if err != nil { return "", nil @@ -458,7 +462,7 @@ func (w *Watcher) addWatch(name string, flags uint32) (string, error) { } // Retry on EINTR; open() can return EINTR in practice on macOS. - // See #354, and go issues 11180 and 39237. + // See #354, and Go issues 11180 and 39237. for { watchfd, err = unix.Open(name, openMode, 0) if err == nil { @@ -491,14 +495,13 @@ func (w *Watcher) addWatch(name string, flags uint32) (string, error) { w.watchesByDir[parentName] = watchesByDir } watchesByDir[watchfd] = struct{}{} - w.paths[watchfd] = pathInfo{name: name, isDir: isDir} w.mu.Unlock() } if isDir { - // Watch the directory if it has not been watched before, - // or if it was watched before, but perhaps only a NOTE_DELETE (watchDirectoryFiles) + // Watch the directory if it has not been watched before, or if it was + // watched before, but perhaps only a NOTE_DELETE (watchDirectoryFiles) w.mu.Lock() watchDir := (flags&unix.NOTE_WRITE) == unix.NOTE_WRITE && @@ -520,13 +523,10 @@ func (w *Watcher) addWatch(name string, flags uint32) (string, error) { // Event values that it sends down the Events channel. func (w *Watcher) readEvents() { defer func() { - err := unix.Close(w.kq) - if err != nil { - w.Errors <- err - } - unix.Close(w.closepipe[0]) close(w.Events) close(w.Errors) + _ = unix.Close(w.kq) + unix.Close(w.closepipe[0]) }() eventBuffer := make([]unix.Kevent_t, 10) diff --git a/fsnotify.go b/fsnotify.go index c00ce762..c230d37d 100644 --- a/fsnotify.go +++ b/fsnotify.go @@ -64,7 +64,7 @@ const ( // Common errors that can be reported. var ( - ErrNonExistentWatch = errors.New("fsnotify: can't remove non-existent watcher") + ErrNonExistentWatch = errors.New("fsnotify: can't remove non-existent watch") ErrEventOverflow = errors.New("fsnotify: queue or buffer overflow") ErrClosed = errors.New("fsnotify: watcher already closed") ) diff --git a/fsnotify_test.go b/fsnotify_test.go index 61ded522..49124adb 100644 --- a/fsnotify_test.go +++ b/fsnotify_test.go @@ -108,7 +108,7 @@ func TestWatch(t *testing.T) { {"file in directory is not readable", func(t *testing.T, w *Watcher, tmp string) { if runtime.GOOS == "windows" { - t.Skip("attributes don't work on Windows") // Figure out how to make a file unreadable + t.Skip("attributes don't work on Windows") // TODO: figure out how to make a file unreadable } touch(t, tmp, "file-unreadable") @@ -160,66 +160,6 @@ func TestWatch(t *testing.T) { }, ` write /file `}, - - {"watch a symlink to a file", func(t *testing.T, w *Watcher, tmp string) { - if runtime.GOOS == "darwin" { - // TODO - // WRITE "/private/var/folders/.../TestWatchwatch_a_symlink_to_a_file183391030/001/file" - // Pretty sure this is caused by the broken symlink-follow - // behaviour too. - t.Skip("broken on macOS") - } - if !internal.HasPrivilegesForSymlink() { - t.Skip("does not have privileges for symlink on this OS") - } - - file := join(tmp, "file") - link := join(tmp, "link") - touch(t, file) - symlink(t, file, link) - addWatch(t, w, link) - - cat(t, "hello", file) - }, ` - write /link - - # TODO: Symlinks followed on kqueue; it shouldn't do this, but I'm - # afraid changing it will break stuff. See #227, #390 - kqueue: - write /file - - # TODO: see if we can fix this. - windows: - empty - `}, - - {"watch a symlink to a dir", func(t *testing.T, w *Watcher, tmp string) { - if runtime.GOOS == "darwin" { - // TODO - // CREATE "/private/var/.../TestWatchwatch_a_symlink_to_a_dir2551725268/001/dir/file" - // Pretty sure this is caused by the broken symlink-follow - // behaviour too. - t.Skip("broken on macOS") - } - if !internal.HasPrivilegesForSymlink() { - t.Skip("does not have privileges for symlink on this OS") - } - - dir := join(tmp, "dir") - link := join(tmp, "link") - mkdir(t, dir) - symlink(t, dir, link) - addWatch(t, w, link) - - touch(t, dir, "file") - }, ` - create /link/file - - # TODO: Symlinks followed on kqueue; it shouldn't do this, but I'm - # afraid changing it will break stuff. See #227, #390 - kqueue: - create /dir/file - `}, } for _, tt := range tests { @@ -277,7 +217,7 @@ func TestWatchCreate(t *testing.T) { // FIFO {"create new named pipe", func(t *testing.T, w *Watcher, tmp string) { if runtime.GOOS == "windows" { - t.Skip() // No named pipes on Windows. + t.Skip("No named pipes on Windows") } touch(t, tmp, "file") addWatch(t, w, tmp) @@ -288,7 +228,7 @@ func TestWatchCreate(t *testing.T) { // Device node {"create new device node pipe", func(t *testing.T, w *Watcher, tmp string) { if runtime.GOOS == "windows" { - t.Skip() // No device nodes on Windows. + t.Skip("No device nodes on Windows") } if isKqueue() { // Don't want to use os/user to check uid, since that pulls in @@ -405,7 +345,7 @@ func TestWatchRename(t *testing.T) { {"rename to unwatched dir", func(t *testing.T, w *Watcher, tmp string) { if runtime.GOOS == "netbsd" && isCI() { - t.Skip("fails in CI; see #488") + t.Skip("fails in CI; see #488") // TODO } unwatched := t.TempDir() @@ -525,6 +465,44 @@ func TestWatchSymlink(t *testing.T) { } tests := []testCase{ + {"watch a symlink to a file", func(t *testing.T, w *Watcher, tmp string) { + file := join(tmp, "file") + link := join(tmp, "link") + touch(t, file) + symlink(t, file, link) + addWatch(t, w, link) + + cat(t, "hello", file) + }, ` + write /link + + # TODO: Symlinks followed on kqueue; it shouldn't do this, but I'm + # afraid changing it will break stuff. See #227, #390 + kqueue: + write /file + + # TODO: see if we can fix this. + windows: + empty + `}, + + {"watch a symlink to a dir", func(t *testing.T, w *Watcher, tmp string) { + dir := join(tmp, "dir") + link := join(tmp, "link") + mkdir(t, dir) + symlink(t, dir, link) + addWatch(t, w, link) + + touch(t, dir, "file") + }, ` + create /link/file + + # TODO: Symlinks followed on kqueue; it shouldn't do this, but I'm + # afraid changing it will break stuff. See #227, #390 + kqueue: + create /dir/file + `}, + {"create unresolvable symlink", func(t *testing.T, w *Watcher, tmp string) { addWatch(t, w, tmp) @@ -539,21 +517,6 @@ func TestWatchSymlink(t *testing.T) { `}, {"cyclic symlink", func(t *testing.T, w *Watcher, tmp string) { - if runtime.GOOS == "darwin" { - // This test is borked on macOS; it reports events outside the - // watched directory: - // - // create "/private/.../testwatchsymlinkcyclic_symlink3681444267/001/link" - // create "/link" - // write "/link" - // write "/private/.../testwatchsymlinkcyclic_symlink3681444267/001/link" - // - // kqueue.go does a lot of weird things with symlinks that I - // don't think are necessarily correct, but need to test a bit - // more. - t.Skip("broken on macOS") - } - symlink(t, ".", tmp, "link") addWatch(t, w, tmp) rm(t, tmp, "link") @@ -571,13 +534,20 @@ func TestWatchSymlink(t *testing.T) { // Bug #277 {"277", func(t *testing.T, w *Watcher, tmp string) { - if isKqueue() { - // TODO: fix it; this seems a bit hard though; the entire way - // kqueue backend deals with symlinks is meh, and need to - // be careful not to break compatibility. - t.Skip("broken on kqueue") + if runtime.GOOS == "netbsd" && isCI() { + t.Skip("fails in CI") // TODO } + // TODO: there is some strange fuckery going on if I use go test + // -count=2; the second test run has unix.Kqueue() in newKqueue() + // return 0, which is a very odd fd number, but the first event does + // work (create /foo). After that we get EBADF (Bad file + // descriptor). + // + // This is *only* for this test, and *only* if we have the symlinks + // below. kqueue(2) doesn't document returning fd 0. + // + // This happens on both FreeBSD and NetBSD. touch(t, tmp, "file1") touch(t, tmp, "file2") symlink(t, join(tmp, "file1"), tmp, "link1") @@ -596,9 +566,17 @@ func TestWatchSymlink(t *testing.T) { rename /apple # mv apple pear create /pear remove /pear # rm -r pear + + # TODO: the CREATE/REMOVE for /pear don't show; I'm not entirely + # sure why; sendDirectoryChangeEvents() doesn't pick it up. It does + # seem consistent though, both locally and in CI. + freebsd, netbsd: + create /foo # touch foo + remove /foo # rm foo + create /apple # mkdir apple + rename /apple # mv apple pear `}, } - for _, tt := range tests { tt := tt tt.run(t) @@ -710,7 +688,6 @@ func TestWatchRemove(t *testing.T) { touch(t, tmp, "e") touch(t, tmp, "f") touch(t, tmp, "g") - mkdir(t, tmp, "h") mkdir(t, tmp, "h", "a") mkdir(t, tmp, "i") @@ -1104,7 +1081,7 @@ func TestClose(t *testing.T) { t.Run("closes channels after read", func(t *testing.T) { if runtime.GOOS == "netbsd" { - t.Skip("flaky") + t.Skip("flaky") // TODO } t.Parallel() @@ -1176,7 +1153,7 @@ func TestAdd(t *testing.T) { t.Run("permission denied", func(t *testing.T) { if runtime.GOOS == "windows" { - t.Skip("chmod doesn't work on Windows") // See if we can make a file unreadable + t.Skip("chmod doesn't work on Windows") // TODO: see if we can make a file unreadable } t.Parallel() @@ -1397,7 +1374,7 @@ func TestEventString(t *testing.T) { // Verify the watcher can keep up with file creations/deletions when under load. func TestWatchStress(t *testing.T) { if isCI() { - t.Skip("fails too often on the CI") + t.Skip("fails too often on the CI") // TODO } // On NetBSD ioutil.ReadDir in sendDirectoryChangeEvents() returns EINVAL