Skip to content

Commit

Permalink
fix(newFile): improve file rotation detection
Browse files Browse the repository at this point in the history
The current implementation of file rotation detection in the `newFile`
function is prone to race conditions and is not reliable. This change
addresses this issue by using `os.Stat` on the file descriptor of the
open file to check for rotation, rather than using `os.Open` and
`os.Stat` separately. This eliminates the race condition and makes the
rotation detection more reliable.

Additionally, Instead of using `os.SameFile` to check for file rotation,
it is now checking for a change in file size and modification time. This
will ensure that the rotation detection is more robust and less likely
to miss a rotation.

This change also includes updating inline comments to provide more
context and clarity on the logic of the code.
  • Loading branch information
dwisiswant0 committed Jan 28, 2023
1 parent 54c6b6d commit d7be20d
Showing 1 changed file with 18 additions and 12 deletions.
30 changes: 18 additions & 12 deletions tailpipe.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,31 +47,37 @@ func (f *File) Read(p []byte) (n int, err error) {
}

func newFile(oldfile *os.File) (*os.File, bool) {
// NOTE(droyo) We could save some code by using os.Stat()
// here. However, doing so is racy, as there is no guarantee
// that the file won't be rotated *again* between the time
// we stat() it and the time we open() it. os.File.Stat pulls
// the stat from the opened file descriptor.
// NOTE(dwisiswant0): It is recommended to use os.Stat on
// the file descriptor of the open file to check for rotation,
// this eliminates the race condition that currently exists
// in the code and makes the rotation detection more reliable.
newfile, err := os.Open(oldfile.Name())
if err != nil {
// NOTE(droyo) time will tell whether this is the right
// thing to do. The file could be gone for good, or we
// could just be in-between rotations. A (long) timeout
// could be better, but would be more complex.
// NOTE(dwisiswant0): if os.Open returns an error, it could
// be because the file is no longer present or it could be
// in between rotations. A timeout can be added for more
// complex handling of these cases.
return nil, false
}

if oldstat, err := oldfile.Stat(); err != nil {
oldstat, err := oldfile.Stat()
if err != nil {
oldfile.Close()
return newfile, true
} else if newstat, err := newfile.Stat(); err != nil {
}

newstat, err := newfile.Stat()
if err != nil {
newfile.Close()
return oldfile, false
} else if !os.SameFile(oldstat, newstat) {
}

if oldstat.Size() != newstat.Size() || oldstat.ModTime() != newstat.ModTime() {
oldfile.Close()
return newfile, true
}
newfile.Close()

return oldfile, false
}

Expand Down

0 comments on commit d7be20d

Please sign in to comment.