Skip to content

Commit

Permalink
Fix symlink behaviour on kqueue
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 Oct 15, 2022
1 parent 36569c7 commit f14d960
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 101 deletions.
37 changes: 20 additions & 17 deletions backend_kqueue.go
View file
Expand Up @@ -363,9 +363,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 @@ -393,35 +394,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 @@ -454,14 +458,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 Down
138 changes: 54 additions & 84 deletions fsnotify_test.go
View file
Expand Up @@ -105,7 +105,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 @@ -157,60 +157,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")
}

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")
}

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 @@ -262,7 +208,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 @@ -273,7 +219,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 @@ -390,7 +336,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 @@ -509,6 +455,44 @@ func TestWatchRename(t *testing.T) {

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 @@ -523,21 +507,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 @@ -555,13 +524,6 @@ 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")
}

touch(t, tmp, "file1")
touch(t, tmp, "file2")
symlink(t, join(tmp, "file1"), tmp, "link1")
Expand All @@ -580,6 +542,14 @@ func TestWatchSymlink(t *testing.T) {
rename /apple # mv apple pear
create /pear
remove /pear # rm -r pear
kqueue:
create /foo # touch foo
remove /foo # rm foo
create /apple # mkdir apple
create /pear # mv apple pear
remove|rename /apple
remove /pear # rm -r pear
`},
}

Expand Down Expand Up @@ -688,7 +658,7 @@ func TestWatchRm(t *testing.T) {

{"remove watched directory", func(t *testing.T, w *Watcher, tmp string) {
if runtime.GOOS == "openbsd" || runtime.GOOS == "netbsd" {
t.Skip("behaviour is inconsistent on OpenBSD and NetBSD, and this test is flaky")
t.Skip("behaviour is inconsistent on OpenBSD and NetBSD, and this test is flaky") // TODO
}

file := join(tmp, "file")
Expand Down Expand Up @@ -871,7 +841,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 @@ -916,7 +886,7 @@ func TestClose(t *testing.T) {
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 @@ -1054,7 +1024,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 f14d960

Please sign in to comment.