Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lock to loglevel changes #3905

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add lock to loglevel changes #3905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8b7326
Add lock to loglevel changes
leaanthony Nov 23, 2024
7973fbf
Update changelog.mdx
leaanthony Nov 23, 2024
83384d1
Merge branch 'master' into v2-bugfix/loglevel-data-race
leaanthony Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions v2/internal/logger/default_logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@ package logger
import (
"fmt"
"os"
"sync"

"github.com/wailsapp/wails/v2/pkg/logger"
)

// LogLevel is an alias for the public LogLevel
type LogLevel = logger.LogLevel

var logLevelLock sync.Mutex
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Move mutex into Logger struct for proper encapsulation

The current implementation has several issues:

  1. Global mutex means all Logger instances share the same lock, causing unnecessary contention
  2. Reading logLevel in other methods (e.g., Debug, Info, etc.) is still not thread-safe
  3. 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.


// Logger is a utlility to log messages to a number of destinations
type Logger struct {
output logger.Logger
Expand Down Expand Up @@ -45,6 +48,8 @@ func (l *Logger) HideLogLevel() {

// SetLogLevel sets the minimum level of logs that will be output
func (l *Logger) SetLogLevel(level LogLevel) {
logLevelLock.Lock()
defer logLevelLock.Unlock()
Comment on lines +51 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

l.logLevel = level
}

Expand Down
2 changes: 1 addition & 1 deletion website/src/pages/changelog.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Fixed example for macOS menu by @takuyahara in [PR](https://github.com/wailsapp/wails/pull/3847)
- Fixed typo by @takuyahara in [PR](https://github.com/wailsapp/wails/pull/3846)
- Fixed incorrect TS definition of `WindowSetSize` by @leaanthony
- Fixed data race in loglevel change by @leaanthony in [PR](https://github.com/wailsapp/wails/pull/3905)
- chore: fix some comments in [PR](https://github.com/wailsapp/wails/pull/3932) by @lvyaoting


### Changed
- Allow to specify macos-min-version externally. Implemented by @APshenkin in [PR](https://github.com/wailsapp/wails/pull/3756)

Expand Down
Loading