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

Test Symlink Add and Remove #388

Closed
wants to merge 1 commit into from
Closed

Test Symlink Add and Remove #388

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
47 changes: 47 additions & 0 deletions integration_test.go
View file
Open in desktop
Expand Up @@ -1076,6 +1076,53 @@ func TestSymlinkNotResolved(t *testing.T) {

}

func TestSymlinkWatchRemoval(t *testing.T) {
testDir := tempMkdir(t)
file1 := filepath.Join(testDir, "file1")
link := filepath.Join(testDir, "link")

f1, err := os.Create(file1)
if err != nil {
t.Fatalf("Failed to create file1: %s", err)
}
defer f1.Close()

// symlink works for Windows too
if err := os.Symlink(file1, link); err != nil {
t.Fatalf("Failed to create symlink: %s", err)
}

w, err := NewWatcher()
if err != nil {
t.Fatalf("Failed to create watcher")
}

// if we AddRaw a symlink
err = w.AddRaw(link)
if err != nil {
t.Fatalf("Failed to add raw symlink: %s", err)
}

// that path name will be watched and can be unwatched
err = w.Remove(link)
if err != nil {
t.Fatalf("Failed to remove link: %s", err)
}

// but if we Add a symlink
err = w.Add(link)
if err != nil {
t.Fatalf("Failed to add link: %s", err)
}

// what should we do if a user tries to Remove it?
err = w.Remove(link)
if err != nil {
t.Fatalf("Failed to remove link: %s", err)
}
Comment on lines +1118 to +1122
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you saying that Remove doesn't follow symlinks? (effectively RemoveRaw)

@Code0x58 What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI tests confirm the issue for Mac and Linux. I had the same problem with my solaris/illumos port.
Windows doesn't seem to care, but Windows also doesn't really have symlinks.

My immediate thought was that a Remove that fails could do an extra check to see if the requested path to remove is a symlink and do the same resolution and try again, but that should probably be implemented in fsnotify.go across all platforms rather than individually in each os-specific implementation.

Definitely curious to know what @Code0x58 thinks we should do.

Copy link
Contributor

@Code0x58 Code0x58 Aug 22, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot and test!

For now, I think renaming Remove to RemoveRaw and making the Windows implementation consistent is the way to go, as well as adding a naive solution similar to Add (i.e. Remove tries to deference the symlinks). The naive Add and Remove would come with caveats to document - I'd probably recommend not using Add/Remove at all unless you know their caveats work in your use case. I'm not sure it would fix any existing users for users, aside from inconsistency with Windows/other implementations.

With a portable low level interface, and documentation on existing issues, the library would be in a better position for users, and may attract contributions towards documenting or implementing common patterns, and maybe even a generic high level interface. A vague comment on a high level interface is here, but it could probably use a lot of attention to avoid leading users in to problematic cases (with races/consistency). If the retry approach would stop a potential issue, and not introduce any more, then I'd say it's a good thing to do so that the nativity/caveats of Add/Remove is incrementally reduced.

ramble: I think the codebase would need a fair bit of fixing and ramping up in testing to offer a properly dependable cross platform solution for non-trivial watching (trivial such as watching a file in a directory which won't be moved or deleted). I say this following experience trying to fix (in #382) the hidden issues found by linting (usually where errors aren't checked in tests, or test objects are used in goroutines which shouldn't - so may not be working at all). That could be a quite a bit of a task taking a lot of attention, but documenting and testing those safe patterns would be good and attract a community. I imagine a fair few people would like a robust pattern for matching a single config file which may be a file, or a symlink to a file such as when you are making a program which may run in or out of a container, and the config may be a projection mount within a container (e.g. when using Docker, Kubernetes, etc.) - that was what inspired my original PR back in 2019.


}

func TestFsnotifyFakeSymlink(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("symlinks don't work on Windows.")
Expand Down