Fix race condition in logging #4519
Conversation
exekias
force-pushed
the
fix-race-logging
branch
from
6fbe842 to
05c0746
Compare
`logp.FileRotator` had an issue when `WriteLines` is called concurrently
exekias
force-pushed
the
fix-race-logging
branch
from
05c0746 to
cdea7f0
Compare
| @@ -73,16 +75,26 @@ func (rotator *FileRotator) WriteLine(line []byte) error { | |||
| } | |||
|
|
|||
| line = append(line, '\n') | |||
|
|
|||
| rotator.currentLock.RLock() | |||
| _, err := rotator.current.Write(line) | |||
There was a problem hiding this comment.
Shouldn't the Write and the increment of the currentSize occur within the same lock block? Without that it seems that the currentSize may not be accurate when it is checked by other goroutines.
There was a problem hiding this comment.
Yeah you are right, I was trying to keep Write under a RLock as it may block for a while, and sacrifice some consistency in currentSize in favor of performance. Putting everything under a write lock would make goroutines logging wait on others, but yeah, this is probably overkill. What do you think?
There was a problem hiding this comment.
I'd say having currentSize slightly out of date is fine, we just use it for the rotator so it's ok if that starts a bit late.
There was a problem hiding this comment.
what do you think @andrewkroh? I can also add some comment explaining this
There was a problem hiding this comment.
I'm OK with the behavior as is.
|
@exekias Does this need back-ported to 5.6? #5131 (comment) |
|
Didn't think about it but it may make sense? |
`logp.FileRotator` had an issue when `WriteLines` is called concurrently (cherry picked from commit fdfcaa3)
Cherry-pick #4519 to 5.6: Fix race condition in logging
logp.FileRotatorhad an issue whenWriteLinesis called concurrently. Added a RWLock to protect bothrotator.currentandrotator.currentSize