Skip to content

Commit

Permalink
Fix symlink behaviour on kqueue (#524)
Browse files Browse the repository at this point in the history 
There were two problems here:

- When we see a link inside a directory the resolved version would get
  added to w.watches etc. but the link won't, resulting in many spurious
  create events for the link. This fixes #277; I'm surprised there
  aren't more reports for this).

- filepath.EvalSymlinks() will resolve any symlinks in the entire path,
  rather than just the link itself. This would cause paths such as:

      /path/to/LINK/dir/dir/WATCH-THIS

  To have the wrong Event.Name; many of the test cases failed because of
  this because /tmp is a link to /private/tmp on macOS, but curiously no
  one reported it AFAIK (I guess many people don't use symlinks all that
  much).

  Example:

      % mkdir /tmp/xxx
      % touch /tmp/xxx/FILE

      % ln -s /tmp/xxx/LINK /tmp/xxx/FILE   # 25 years of Unix experience, and still...
      ln: /tmp/xxx/FILE: File exists
      % ln -s /tmp/xxx/FILE /tmp/xxx/LINK

  Before it would do:

      % go run ./cmd/fsnotify watch /tmp/xxx &
      % touch /tmp/xxx/FILE
      03:23:03.2731   1 CHMOD         "/private/tmp/xxx/FILE"
      03:23:03.2744   2 CHMOD         "/tmp/xxx/FILE"
      ^C

      % fsnotify watch /tmp/xxx/LINK &
      % touch /tmp/xxx/LINK
      03:26:37.3576   1 CHMOD         "/private/tmp/xxx/FILE"

  And now it does:

      % go run ./cmd/fsnotify watch /tmp/xxx &
      03:23:47.8911   1 CHMOD         "/tmp/xxx/FILE"
      ^C

      % fsnotify watch /tmp/xxx/LINK &
      % touch /tmp/xxx/LINK
      03:27:38.5227   1 CHMOD         "/tmp/xxx/FILE"
  • Loading branch information
@arp242
arp242 committed Jul 12, 2023
1 parent 97fdd1d commit c35de00
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 114 deletions.
12 changes: 10 additions & 2 deletions CHANGELOG.md
View file
Expand Up @@ -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
Expand Down
44 changes: 22 additions & 22 deletions backend_kqueue.go
View file
Expand Up @@ -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)
Expand Down Expand Up @@ -430,35 +431,38 @@ 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
}
}

// 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 {
Expand Down Expand Up @@ -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 &&
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion fsnotify.go
View file
Expand Up @@ -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")
)
Expand Down
155 changes: 66 additions & 89 deletions fsnotify_test.go
View file
Expand Up @@ -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")
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)

Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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)
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit c35de00

Please sign in to comment.