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
Fixes inotify not notifying on deletion of open files. #205
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 :)
|
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.
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
// Otherwise the event is ignored. | ||
// *Note*: this was put in place because it was seen that a MODIFY | ||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo, should be triggered
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.
Lets also link to this issue in the comment.
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.
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. |
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.
Lets add a comment onto the Watcher struct to document this.
@@ -447,3 +447,63 @@ func TestInotifyOverflow(t *testing.T) { | |||
numDirs*numFiles, creates) | |||
} | |||
} | |||
|
|||
func TestInotifyDeleteOpenFile(t *testing.T) { |
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.
should have t.Parallel()
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
Chmod
notification 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_ATTRIB
notification 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_SELF
notifications will be postponed until then. With this change, a watcher process will receive aChmod
notification 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.