Fixes inotify not notifying on deletion of open files.#205
Fixes inotify not notifying on deletion of open files.#205vladlosev wants to merge 5 commits intofsnotify:masterfrom
Conversation
|
Is there something blocking the merging of this? It fixes an issue in a utility I'm maintaining, and seems to pass all tests etc? |
|
Just no one has reviewed yet. |
|
Ah, ok. I guess my review isn’t worth much, as I don’t really understand the potential breakages from this change... But for what observable behaviour is worth, it does fix problem and doesn’t seem to cause any new ones :)
|
nhooyr
left a comment
There was a problem hiding this comment.
Why is sending the Chmod event what we want to do here? How could it be used by apps to circumvent the issue described #205? E.g. how would you know the Chmod is because the file has been deleted? Would the caller be have to check whether the file exists on every Chmod?
inotify.go
Outdated
| // event was sent after the DELETE. This ignores that MODIFY and | ||
| // assumes a DELETE will come or has come if the file doesn't exist. | ||
| if !(e.Op&Remove == Remove || e.Op&Rename == Rename) { | ||
| // A CHMOD event (triggeded by IN_MODIFY) can arrive when an open file is |
There was a problem hiding this comment.
Lets also link to this issue in the comment.
There was a problem hiding this comment.
Added the issue link and fixed the typo.
3da4b91 to
877935a
Compare
|
@nhooyr I have outlined the alternatives to fix the issue in #194 (comment). Sending the Chmod event is the simplest solution. Yes, it will require checking for file existence upon receiving the |
Why don't we go with this but ignore the extra Remove notification? Then fsnotify will behave the way it does everywhere else. |
|
It's possible but then we start to get into corner cases such as when the file is moved and then has attributes changed. Handling that will either require more complex (and thus error prone) logic to track file renames or we'll be losing the remove notification. |
|
After a file is moved, the watcher will not receive any events for it under the old path afaik. So we should be good. |
|
@vladlosev thank you for the contribution. It's greatly appreciated. I need this in my own project so do you mind if I take this over? |
|
Actually my solution would not work as if a rename happens following a chmod, we would potentially replace the chmod with a delete whereas we should not have done that. I'm not convinced there is a good solution for this. I think this is a limitation/mistake in the inotify API. However, your solution is better than completely ignoring it as, as you said, it at least notifies the client that something has happened. |
nhooyr
left a comment
There was a problem hiding this comment.
Lets add a comment onto the Watcher struct to document this.
| } | ||
| } | ||
|
|
||
| func TestInotifyDeleteOpenFile(t *testing.T) { |
|
Otherwise, LGTM. |
|
@nhooyr feel free to finish this the best way you see fit. |
|
@vladlosev Thanks very much for your work on this. It looks like #260 is based on what you've done. Please take a look at that if you have time. |
|
thanks |
What does this pull request do?
Fixes issue #194: no notification arrives when a file with open handles is deleted. The change essentially stops ignoring the
Chmodnotification when the file in question does not exist. Such situation can arise on Linux, when a process holds an open handle for the file and the file is being deleted. In that case the watcher receives anIN_ATTRIBnotification to indicate the changed count of hard links to the file. But the file will not be actually deleted from disk until all open handles are closed, and correspondinglyIN_DELETE_SELFnotifications will be postponed until then. With this change, a watcher process will receive aChmodnotification and have a chance to respond.Where should the reviewer start?
The test explains the scenario well and can be a good place to understand it better.
How should this be manually tested?
A unit test is included and is executed as part of the CI build. The test fails without the code change.