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.
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
Test Symlink Add and Remove #388
Conversation
// 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) | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
What does this pull request do?
Adds a test case to highlight the confusion about symlinks in #387
Where should the reviewer start?
Please look at the test failures and the code and provide guidance.
How should this be manually tested?
go test -v -run TestSymlinkWatchRemoval