Fix race conditions in logging package functions and enable race detection in tests #1101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maiqueb
approved these changes
Jun 6, 2023
pkg/logging/logging.go
Outdated
| } | ||
|
|
||
| func init() { | ||
| loggingStderr = true | ||
| loggingW = nil | ||
| loggingLevel = PanicLevel | ||
| logger = &lumberjack.Logger{} | ||
|
|
There was a problem hiding this comment.
my bad missed it :) i removed it
AlinaSecret
force-pushed
the
fix-race-condition
branch
from
92fd0d0
to
24a2911
Compare
maiqueb
reviewed
Jun 7, 2023
pkg/logging/logging.go
Outdated
Comment on lines
60
to
61
| var updatedLogger lumberjack.Logger = lumberjack.Logger{Filename: logger.Filename, MaxAge: logger.MaxAge, MaxBackups: logger.MaxBackups, | ||
| Compress: logger.Compress, MaxSize: logger.MaxSize, LocalTime: logger.LocalTime} |
There was a problem hiding this comment.
sorry for not raising it before, but do you really need to initialize these ? You're over-writing the defaults just below.
I mean, couldn't you just:
var updatedLogger lumberjack.Logger = lumberjack.Logger{
Filename: logger.Filename,
MaxAge: 5,
MaxBackups: 5,
Compress: true,
MaxSize: 100,
LocalTime: logger.LocalTime
}
if options != nil {
...
}
...There was a problem hiding this comment.
glad you raised it, changed as you suggested :)
maiqueb
reviewed
Jun 7, 2023
pkg/logging/logging.go
Outdated
Comment on lines
180
to
181
| var updatedLogger lumberjack.Logger = lumberjack.Logger{Filename: filename, MaxAge: logger.MaxAge, MaxBackups: logger.MaxBackups, | ||
| Compress: logger.Compress, MaxSize: logger.MaxSize, LocalTime: logger.LocalTime} |
There was a problem hiding this comment.
please use one attribute per line
AlinaSecret
force-pushed
the
fix-race-condition
branch
from
24a2911
to
9d64562
Compare
|
Oh and thanks for the PR! @AlinaSecret Good catch :) |
The logging package contains two functions, SetLogOptions and SetLogFile, that could experience race conditions when multiple goroutines access and modify the logger struct concurrently. To address these issues, a copy of the logger struct is now created in each function to eliminate data races. In addition, the test-go.sh script is updated to include the '-race' flag, enabling race detection during testing. This change helps prevent future race conditions by activating the Go race detector. Signed-off-by: Alina Sudakov <[email protected]>
AlinaSecret
force-pushed
the
fix-race-condition
branch
from
9d64562
to
272b3ca
Compare
dougbtv
approved these changes
Aug 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the race conditions discovered during local testing using the Go race command. The race command detected potential data race conditions in the logging package's SetLogOptions and SetLogFile functions when multiple goroutines accessed and modified the shared logger struct concurrently.
Before applying the fix, the race detector logs indicated conflicting accesses as can be seen here:
To resolve these race conditions, a copy of the logger struct is now created within each function.
This PR enables race detection in unit tests by modifying the the test-go.sh script . The test configuration is updated to include the '-race' flag, allowing thorough race detection during testing.
for further reference about race detection view: https://go.dev/doc/articles/race_detector)