inotify: remove watch when renaming a watched path #518
Conversation
Renaming a watched path is problematic; on inotify we just get a IN_MOVED_SELF event with the old filename and that's it; no more events for you! So if you do: watch one mv one two cat asd >two You still continue to get events for the file "one", even though it's now named "two" (the file descriptor doesn't care about the rename). There is no way we can know the new event as far as I can tell, inotifywait(1) also behaves like this. So instead of continuing in a semi-broken state just remove the watcher, like we do for deletes. On kqueue and FEN the situation is similar, and we actually already removed watchers on renames. On Windows this all works nicely; the watch is preserved and the filename is updated. I decided to keep this as-is for now, even though it's inconsistent. We actually fixed the Windows behaviour for the 1.6.0 release in #370 , so people do seem to care about it and use it, and experience with the symlink change in 1.5.0 shows it's better to keep inconsistent behaviour rather than change it. Fixing this up is something for a v2. Fixes #172 Fixes #503
nshalman
left a comment
nshalman
left a comment
There was a problem hiding this comment.
One nit on a comment.
Between the tests passing and my review, I think we're good.
I do think I have a pretty good handle on the changes you made to the tests, but all the same I'd prefer if someone else with more inotify experience approved too.
That said, if folks are busy, I wouldn't wait an inordinate amount of time for a second review.
You're basically the only person regularly doing reviews at the moment, so ... 🙃 Even at $dayjob getting people to review things in a vaguely timely manner has often been a challenge. My general approach has been "just merge it if I'm fairly confident it's good". People can always comment later, or we can fix things later, or even backout something if need be. I tend to work on these kind of things in "bursts", e.g. a lot for a week and then nothing for a bit while I do other stuff. I don't really expect people to review >10 PRs in a week for a project like this. I think I will just stop asking them except for the larger changes like #521 that change the API; technical fixes like this one are easy to prove correct, but API changes and the like are a more subjective matter of taste, and it's good to have some people look at that, which is more important than stuff like this. That would reduce the requests, improving the chance people actually look at it. |
Release 1.7.0 This version of fsnotify needs Go 1.17. Additions: - illumos: add FEN backend to support illumos and Solaris. ([fsnotify#371]) - all: add `NewBufferedWatcher()` to use a buffered channel, which can be useful in cases where you can't control the kernel buffer and receive a large number of events in bursts. ([fsnotify#550], [fsnotify#572]) - all: add `AddWith()`, which is identical to `Add()` but allows passing options. ([fsnotify#521]) - windows: allow setting the ReadDirectoryChangesW() buffer size with `fsnotify.WithBufferSize()`; the default of 64K is the highest value that works on all platforms and is enough for most purposes, but in some cases a highest buffer is needed. ([fsnotify#521]) Changes and fixes: - inotify: remove watcher if a watched path is renamed ([fsnotify#518]) After a rename the reported name wasn't updated, or even an empty string. Inotify doesn't provide any good facilities to update it, so just remove the watcher. This is already how it worked on kqueue and FEN. On Windows this does work, and remains working. - windows: don't listen for file attribute changes ([fsnotify#520]) File attribute changes are sent as `FILE_ACTION_MODIFIED` by the Windows API, with no way to see if they're a file write or attribute change, so would show up as a fsnotify.Write event. This is never useful, and could result in many spurious Write events. - windows: return `ErrEventOverflow` if the buffer is full ([fsnotify#525]) Before it would merely return "short read", making it hard to detect this error. - kqueue: make sure events for all files are delivered properly when removing a watched directory ([fsnotify#526]) Previously they would get sent with `""` (empty string) or `"."` as the path name. - kqueue: don't emit spurious Create events for symbolic links ([fsnotify#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 ([fsnotify#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 WASM, AIX, etc. ([fsnotify#528]) - other: use the `backend_other.go` no-op if the `appengine` build tag is set; Google AppEngine forbids usage of the unsafe package so the inotify backend won't compile there. [fsnotify#371]: fsnotify#371 [fsnotify#516]: fsnotify#516 [fsnotify#518]: fsnotify#518 [fsnotify#520]: fsnotify#520 [fsnotify#521]: fsnotify#521 [fsnotify#524]: fsnotify#524 [fsnotify#525]: fsnotify#525 [fsnotify#526]: fsnotify#526 [fsnotify#528]: fsnotify#528 [fsnotify#537]: fsnotify#537 [fsnotify#550]: fsnotify#550 [fsnotify#572]: fsnotify#572
Renaming a watched path is problematic; on inotify we just get a IN_MOVED_SELF event with the old filename and that's it; no more events for you! So if you do:
You still continue to get events for the file "one", even though it's now named "two" (the file descriptor doesn't care about the rename). There is no way we can know the new event as far as I can tell, inotifywait(1) also behaves like this. So instead of continuing in a semi-broken state just remove the watcher, like we do for deletes.
On kqueue and FEN the situation is similar, and we actually already removed watchers on renames.
On Windows this all works nicely; the watch is preserved and the filename is updated. I decided to keep this as-is for now, even though it's inconsistent. We actually fixed the Windows behaviour for the 1.6.0 release in #370 , so people do seem to care about it and use it, and experience with the symlink change in 1.5.0 shows it's better to keep inconsistent behaviour rather than change it. Fixing this up is something for a v2.
Fixes #172
Fixes #503