Prevent file and goroutine leaks during self-hosted log tailing - #828
Conversation
| out <- SelfHostedLogStreamItem{Line: line.String()} | ||
| case line, ok := <-t.Lines(): | ||
| if !ok { | ||
| break TailLoop |
There was a problem hiding this comment.
The Lines channel could close due to an internal error. In that case we should break out of the loop to free the file descriptor
| } | ||
| select { | ||
| case out <- SelfHostedLogStreamItem{Line: line.String()}: | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
Check for context cancellation before writing to the out channel to avoid a possible deadlock
There was a problem hiding this comment.
Makes sense especially since that channel is currently unbuffered (and it seems beneficial to keep that, or at most use a low capacity, so that we backpressure on the actual file read in go-tail to avoid high memory use with large log lines).
| err = watcher.Add(logLocation) | ||
| if err != nil { | ||
| watcher.Close() | ||
| return fmt.Errorf("fsnotify add \"%s\": %s", logLocation, err) |
There was a problem hiding this comment.
watcher.Add is now before the goroutine launch so Add failures don't orphan the watcher goroutine and its open file tails.
| } | ||
| } | ||
| if event.Op&fsnotify.Remove == fsnotify.Remove || event.Op&fsnotify.Rename == fsnotify.Rename || event.Op&fsnotify.Chmod == fsnotify.Chmod { | ||
| if event.Op&fsnotify.Remove == fsnotify.Remove || event.Op&fsnotify.Rename == fsnotify.Rename { |
There was a problem hiding this comment.
Not related to the rest of the changes, but I'm not sure it makes sense to close the tail if the permissions change. At the very least we can end up losing in-progress logs.
| func (t *Follower) Close() { | ||
| if t.file != nil { | ||
| t.file.Close() | ||
| } | ||
| t.closeCh <- struct{}{} | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
It looks like this change is not actually safe to do, since it causes a data race between Go routines (since we're calling t.file.Close() from a different Go routine than the one that actually manages the file), as observed in https://github.com/pganalyze/collector/actions/runs/29125918855/job/86472202020?pr=827
If I recall our conversation correctly, this eager closing was actually not strictly required, since making the channel buffered addresses the Go routine getting stuck. I've dug a bit into this, and revised that change further in pganalyze/go-tail#1 and also made a PR to update to that: #836
seanlinsley
changed the title
2 participants
No description provided.