Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix symlink behaviour on kqueue #524

Merged
merged 1 commit into from
Jul 12, 2023
Merged

Fix symlink behaviour on kqueue #524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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