Conversation
Allow to close the underlying file.
| defer func() { | ||
| logger.Close() | ||
| }() |
There was a problem hiding this comment.
Just defer logger.Close()?
There was a problem hiding this comment.
Actually maybe we should be printing the error to stderr
log/rotate.go
Outdated
| for range ch { | ||
| if err := w.Reopen(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "failed to rotate log file: %v\n", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure if this goroutine ends. What closes the channel?
There was a problem hiding this comment.
How about we close it in RotatableFile.Close()?
|
Let's comment that forwarder supports SIGHUP log rotation in --log-file flag usage. |
log/rotate.go
Outdated
| select { | ||
| case w.ch <- closeSignal: | ||
| default: | ||
| } |
There was a problem hiding this comment.
I understand, it is to protect consecutive Close calls, but what If the channel is occupied with SIGHUPs and you only call close once. It's a rare case, but not impossible.
There was a problem hiding this comment.
This protects against blocking on Close() when Close is called multiple times after to Goroutine exited.
Note there is a race condition between Close and Reopen (and write) using atomics.
We could solve it by locking but then all logging would go under RW lock read path.
Given the use case I do not think it's a right approach.
There is not much we can do here tbh given this design - that's why there was no cancelation in the 1st place - it's a virtual problem.
Some of the common aids do not work here
- Closing the channel causes panics in signal and Subsequent calls to Close
- Using separate channel has the same problem
- Increasing channel buffer size does not work as you can have multiple signals queued in theory
Maybe you can suggest something that I'm missing.
There was a problem hiding this comment.
How about use sync.Once to send closeSignal
Add RotatableFile that allows for file reopening files on SIGHUP. This allows Forwarder to work with tools like logrotate.
No description provided.