Add lock to loglevel changes #3905
Conversation
WalkthroughThe changes introduce a mutex lock to the Changes
Assessment against linked issues
Warning Rate limit exceeded@leaanthony has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)website/src/pages/changelog.mdx (1)
The entry correctly documents the fix for the data race issue in log level changes, providing proper attribution and PR reference. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying wails with
|
| Latest commit: |
7973fbf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c5636f26.wails.pages.dev |
| Branch Preview URL: | https://v2-bugfix-loglevel-data-race.wails.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
v2/internal/logger/default_logger.go (1)
Thread safety concerns require attention in logger implementations
The verification revealed several critical thread-safety issues:
- The
FileLoggerimplementation opens and closes file handles on every write operation without synchronization, which is unsafe for concurrent access.- The default logger uses
printlnwhich isn't guaranteed to be thread-safe.- The original concern about
logLevelaccess is valid, but is less critical compared to the concurrent write issues.Specific issues that need addressing:
v2/pkg/logger/filelogger.go: Add mutex to protect file operationsv2/pkg/logger/default.go: Replaceprintlnwith a thread-safe alternativev2/internal/logger/default_logger.go: ProtectlogLevelaccess as originally noted🔗 Analysis chain
Line range hint
1-124: Consider comprehensive thread safety improvementsSeveral architectural improvements would enhance thread safety:
- All methods reading
logLevelneed synchronization (Trace, Debug, Info, Warning, Error)- Consider using atomic operations for logLevel if performance is critical
- The Fatal method's
os.Exitcall could interrupt other goroutines mid-operationLet's verify if there are any other goroutine-unsafe operations in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other logger instances that might have similar issues rg -l "type.*Logger.*struct" --type go # Search for goroutine usage with logging rg "go\s+func.*\blog[\.|\(]" --type goLength of output: 281
Script:
#!/bin/bash # Let's examine the logger implementations to understand thread safety patterns rg -A 5 "type.*Logger.*struct" v2/pkg/logger/default.go v2/pkg/logger/filelogger.go v2/pkg/clilogger/clilogger.go # Check for mutex usage in logger implementations rg "sync\..*Mutex" v2/pkg/logger/default.go v2/pkg/logger/filelogger.go v2/pkg/clilogger/clilogger.go # Look for concurrent logging calls rg "go\s+.*\.(Debug|Info|Warning|Error|Fatal)" --type go # Check custom logger implementation cat v2/internal/logger/custom_logger.goLength of output: 3892
Script:
#!/bin/bash # Let's check the actual logger interface and implementations cat v2/pkg/logger/logger.go cat v2/pkg/logger/default.go cat v2/pkg/logger/filelogger.go # Check for any concurrent write operations to files rg "Write|Print" v2/pkg/logger/filelogger.go -A 2Length of output: 4178
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
v2/internal/logger/default_logger.go(2 hunks)
🔇 Additional comments (1)
v2/internal/logger/default_logger.go (1)
6-6: LGTM: Necessary import for mutex implementation
| logLevelLock.Lock() | ||
| defer logLevelLock.Unlock() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update locking mechanism to use struct-level mutex
The locking mechanism is correct, but needs to be updated to use the struct-level mutex.
func (l *Logger) SetLogLevel(level LogLevel) {
- logLevelLock.Lock()
- defer logLevelLock.Unlock()
+ l.logLevelLock.Lock()
+ defer l.logLevelLock.Unlock()
l.logLevel = level
}Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| "github.com/wailsapp/wails/v2/pkg/logger" | ||
| ) | ||
|
|
||
| // LogLevel is an alias for the public LogLevel | ||
| type LogLevel = logger.LogLevel | ||
|
|
||
| var logLevelLock sync.Mutex |
There was a problem hiding this comment.
Critical: Move mutex into Logger struct for proper encapsulation
The current implementation has several issues:
- Global mutex means all Logger instances share the same lock, causing unnecessary contention
- Reading
logLevelin other methods (e.g., Debug, Info, etc.) is still not thread-safe - Poor encapsulation as the lock should be part of the Logger struct
Apply these changes to fix the issues:
type Logger struct {
output logger.Logger
logLevel LogLevel
showLevelInLog bool
+ logLevelLock sync.RWMutex // Use RWMutex for better performance
}
-var logLevelLock sync.Mutex
func New(output logger.Logger) *Logger {
if output == nil {
output = logger.NewDefaultLogger()
}
result := &Logger{
logLevel: logger.INFO,
showLevelInLog: true,
output: output,
}
return result
}Then update all methods that read logLevel to use RLock():
func (l *Logger) Debug(format string, args ...interface{}) {
+ l.logLevelLock.RLock()
+ level := l.logLevel
+ l.logLevelLock.RUnlock()
- if l.logLevel <= logger.DEBUG {
+ if level <= logger.DEBUG {
l.output.Debug(fmt.Sprintf(format, args...))
}
}Committable suggestion skipped: line range outside the PR's diff.
Description
Ensures log level changes don't cause a data race.
Fixes #2493
Summary by CodeRabbit
WindowSetSize.webview2loaderissue.