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

Any file changes that occurred before InotifyFileWatcher was started but after the preceding EOF would not be detected #1

Merged
merged 2 commits into from
Aug 31, 2021
Merged

Any file changes that occurred before InotifyFileWatcher was started but after the preceding EOF would not be detected #1

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba36bda
File watcher may not capture any changes that occured before the watc…
theencee Aug 27, 2021
a6a5624
review comments & gofmt
theencee Aug 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions watch/inotify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ func (fw *InotifyFileWatcher) ChangeEvents(t *tomb.Tomb, pos int64) (*FileChange

events := Events(fw.Filename)

// Check to see the file hasn't been modified already before the watcher started
fi, deleted := fw.checkAndNotifyIfModifiedInBetween(changes)
if deleted {
return
}
fw.Size = fi.Size()

for {
prevSize := fw.Size

Expand Down Expand Up @@ -134,3 +141,27 @@ func (fw *InotifyFileWatcher) ChangeEvents(t *tomb.Tomb, pos int64) (*FileChange

return changes, nil
}

func (fw *InotifyFileWatcher) checkAndNotifyIfModifiedInBetween(changes *FileChanges) (os.FileInfo, bool) {
fi, err := os.Stat(fw.Filename)
if err != nil {
if os.IsNotExist(err) {
RemoveWatch(fw.Filename)
changes.NotifyDeleted()
return nil, true
}
util.Fatal("Failed to stat file %v: %v", fw.Filename, err)
Copy link

Choose a reason for hiding this comment

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

It seems in line with the library, but I find it highly concerning if I can make a program crash by concurrently removing read access from the directory containing a watched file :/

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense even though it wouldn't apply to our use case. There are even comments like: // XXX: report this error back to the user in the code which I highly doubt would ever actually be fixed. In this case I think we should log an error and treat it as a deleted file

}

if fw.Size > 0 && fw.Size > fi.Size() { // old file size was larger than now => truncated
Copy link

Choose a reason for hiding this comment

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

Can fi.Size() ever be negative? If not, fw.Size() > 0 is redundant

Copy link
Author

Choose a reason for hiding this comment

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

I can't imagine os.Stat would return a negative file size. Removed

changes.NotifyTruncated()
} else if fw.Size != fi.Size() {
changes.NotifyModified()
}
// there is a corner case of file that was truncated and replaced with exact same amount of bytes, which would
// result in no notification. However any subsequent writes will capture that
// If the file isn't expected to be written to often and those events cannot be missed, recommend using Polling watcher
// instead of inotify

return fi, false
}