fix logger race #1650
fix logger race #1650
Conversation
WalkthroughThe pull request introduces modifications to the Go-MySQL-Driver's logging mechanism, focusing on enhancing thread safety and logger management. The changes involve replacing the static Changes
Sequence DiagramsequenceDiagram
participant Code as Application Code
participant Driver as MySQL Driver
participant Logger as Atomic Logger
Code->>Driver: Initialize/Use Driver
Driver->>Logger: getLogger()
Logger-->>Driver: Current Logger Instance
Driver->>Logger: Perform Logging
Note over Logger: Thread-safe Logger Access
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dsn.go (1)
206-206: LGTM! Consider adding documentation about thread-safety.The change to use
getLogger()in the normalize method is correct. Consider adding a comment explaining that this ensures thread-safe logger initialization.if cfg.Logger == nil { + // Use thread-safe logger initialization cfg.Logger = getLogger() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
AUTHORS(1 hunks)driver_test.go(1 hunks)dsn.go(2 hunks)dsn_test.go(1 hunks)errors.go(3 hunks)errors_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- AUTHORS
- driver_test.go
🔇 Additional comments (6)
errors.go (2)
41-41: LGTM: Good use of atomic.Pointer for thread safety
The change from a static logger to atomic.Pointer[Logger] is a good solution for preventing race conditions in concurrent access.
43-45: LGTM: Proper initialization in init function
The init function correctly sets up the default logger with appropriate configuration.
errors_test.go (1)
Line range hint 19-31: LGTM: Tests properly updated for new logger implementation
The test changes correctly use the new getLogger() function and properly clean up the logger state after the test.
dsn_test.go (2)
20-87: LGTM: Test data properly updated for new logger implementation
The test data structure has been correctly updated to use getLogger() consistently across all test cases.
41-75: Verify all logger usages are updated
Let's ensure all references to the old defaultLogger have been updated to use getLogger().
✅ Verification successful
Let me analyze the findings:
- The
defaultLoggeris only used inerrors.gofor storage and loading operations, which appears to be the intended implementation for managing the logger. - All logger initializations in the codebase are using
getLogger()consistently, including:- All test cases in
dsn_test.go - Default configuration in
dsn.go - Test setup in
driver_test.go
- All test cases in
Logger usage verification complete
All logger usages have been properly updated. The only occurrences of defaultLogger are in errors.go where it's used as the storage mechanism, which is the correct implementation. All other parts of the codebase consistently use getLogger() for logger initialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct uses of defaultLogger that haven't been updated
# Search for direct defaultLogger usage
rg "defaultLogger\." --type go
# Search for logger initialization patterns
rg "Logger\s*[:=]" --type go
Length of output: 5368
dsn.go (1)
92-92: LGTM! Verify thread-safe logger initialization.
The change from defaultLogger to getLogger() aligns with the PR's goal of fixing race conditions in logger management.
Let's verify the thread-safety of the new logger implementation:
✅ Verification successful
Let me verify the implementation of getLogger() and the atomic logger to ensure thread safety.
Thread-safe logger implementation confirmed
The implementation is correctly using atomic operations for logger access:
defaultLoggeris properly declared asatomic.Pointer[Logger]getLogger()safely retrieves the logger using atomicLoad()- All logger access points consistently use
getLogger()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the thread-safety implementation of getLogger()
# Expected: Find the atomic.Pointer[Logger] declaration and all logger access points
# Check for atomic logger implementation
rg -l "atomic\.Pointer\[Logger\]"
# Find all logger access points to ensure consistent usage of getLogger()
ast-grep --pattern 'getLogger()'
Length of output: 5373
Script:
#!/bin/bash
# Check the implementation of getLogger() and the atomic logger
rg -A 5 -B 5 "func getLogger\(\)"
rg -A 5 -B 5 "atomic\.Pointer\[Logger\]"
Length of output: 732
|
Is it real problem? |
Sure. If you run tests of your service using this driver with SetLogger and -race flag tests will fail For example: |
|
We assume SetLogger is called once before Calling SetLogger during using connector doesn't seem valid use case. |
|
Then I don't understand why the SetLogger method is needed :) |
As I wrote once, it can be used to set logger before using the driver. |
|
Ah, ok, thanks) |
Description
Hello brothers :) I've fixed a race condition in the SetLogger method.
I replaced defaultLogger with an atomic.Pointer[Logger].
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests