fix(init): conn init should be thread safe #3555
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves thread safety for connection initialization by adding a new initializing flag to prevent concurrent initialization attempts. The changes introduce synchronized access to initialization state and proper cleanup on initialization failures.
Key changes:
- Adds an
initializingatomic flag to track in-progress initialization - Implements deferred cleanup to reset state on initialization failure
- Adds retry logic with timeout when replacing network connections
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| redis.go | Adds initialization guard with deferred cleanup and reorders initialization steps for thread safety |
| internal/pool/conn.go | Introduces initializing flag with accessor methods and adds retry logic for network connection replacement |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| time.Sleep(100 * time.Millisecond) | ||
| maxRetries-- | ||
| if maxRetries <= 0 { | ||
| return fmt.Errorf("failed to set net conn after %d attempts due to high contention", maxRetries) |
There was a problem hiding this comment.
The error message uses maxRetries which will always be 0 or negative at this point. Use the original value (20) or calculate the actual number of attempts made instead.
| return fmt.Errorf("failed to set net conn after %d attempts due to high contention", maxRetries) | |
| return fmt.Errorf("failed to set net conn after %d attempts due to high contention", 20) |
| // max retries of 100ms * 20 = 2 second | ||
| maxRetries := 20 | ||
| for cn.IsInitializing() || cn.IsUsed() { | ||
| time.Sleep(100 * time.Millisecond) | ||
| maxRetries-- | ||
| if maxRetries <= 0 { | ||
| return fmt.Errorf("failed to set net conn after %d attempts due to high contention", maxRetries) | ||
| } | ||
| } |
There was a problem hiding this comment.
The retry loop blocks the goroutine with fixed 100ms sleeps, which could cause unnecessary delays. Consider using exponential backoff or a more efficient synchronization mechanism like a condition variable.
|
|
||
| // SetNetConnAndInitConn replaces the underlying connection and executes the initialization. | ||
| func (cn *Conn) SetNetConnAndInitConn(ctx context.Context, netConn net.Conn) error { | ||
| // max retries of 100ms * 20 = 2 second |
There was a problem hiding this comment.
Corrected 'second' to 'seconds' for grammatical accuracy.
| // max retries of 100ms * 20 = 2 second | |
| // max retries of 100ms * 20 = 2 seconds |
|
closed in favour of #3559 |
conn init should be thread safe