-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix(conn): conn to have state machine #3559
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors connection management to use a state machine architecture, replacing the previous boolean flags (usable, Inited, used) with a formal state machine that provides better concurrency control and clearer state transitions.
Key Changes:
- Introduced a new
ConnStateMachinewith explicit states: CREATED → INITIALIZING → IDLE ⇄ IN_USE, with UNUSABLE for handoff/reauth operations - Replaced atomic boolean flags with atomic state transitions using CAS operations and FIFO waiting queues
- Updated connection initialization logic to handle concurrent initialization attempts properly
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
internal/pool/conn_state.go |
New file implementing the connection state machine with FIFO waiting queue |
internal/pool/conn_state_test.go |
Comprehensive tests for state machine transitions and concurrency |
internal/pool/conn.go |
Refactored connection to use state machine instead of boolean flags |
internal/pool/pool.go |
Updated pool operations to use state machine transitions |
internal/pool/pool_single.go |
Added comments explaining SingleConnPool's non-thread-safe nature |
redis.go |
Refactored initConn to handle state transitions and concurrent initialization |
maintnotifications/pool_hook.go |
Updated to check handoff state before usability |
maintnotifications/pool_hook_test.go |
Updated tests to use state machine API |
maintnotifications/handoff_worker.go |
Added call to clear handoff state before closing |
maintnotifications/errors.go |
Added new error type and documentation comments |
internal/auth/streaming/pool_hook.go |
Simplified reauth logic using state machine's AwaitAndTransition |
internal/auth/streaming/pool_hook_state_test.go |
New tests verifying reauth respects state machine |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
810c129 to
4876996
Compare
4876996 to
600dfe2
Compare
4a452b0 to
b55cdad
Compare
b55cdad to
fc2da24
Compare
1642bb5 to
a7b2f91
Compare
a7b2f91 to
d207749
Compare
68199f2 to
d91800d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 34 out of 37 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| It("should initialize idle connections created by MinIdleConns", Label("NonRedisEnterprise"), func() { | ||
| opt := redisOptions() | ||
| passwrd := "asdf" |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'passwrd' to 'password'.
| var sysErr error | ||
|
|
||
| if err := rawConn.Read(func(fd uintptr) bool { | ||
| if err := rawConn.Control(func(fd uintptr) { |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed from rawConn.Read() to rawConn.Control(), but the function signature changed from returning bool to void. The return true statement was removed on line 48, which is correct. However, Control() should be used for operations that don't read/write, while Read() is for read operations. Using Recvfrom with MSG_PEEK is a read operation, so rawConn.Read() was semantically correct. Consider reverting to Read() or document why Control() is preferred here.
| if err := rawConn.Control(func(fd uintptr) { | |
| if err := rawConn.Read(func(fd uintptr) { |
| if opt.MaxActiveConns < opt.PoolSize { | ||
| opt.MaxActiveConns = opt.PoolSize | ||
| } |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 163-164 contain redundant condition checking. Line 163 already checks opt.MaxActiveConns < opt.PoolSize, so the inner if on line 164 will always be true. This appears to be dead code or a logic error.
| if opt.MaxActiveConns < opt.PoolSize { | |
| opt.MaxActiveConns = opt.PoolSize | |
| } |
| func (cn *Conn) TryAcquire() bool { | ||
| // The || operator short-circuits, so only 1 CAS in the common case | ||
| return cn.stateMachine.state.CompareAndSwap(uint32(StateIdle), uint32(StateInUse)) || | ||
| cn.stateMachine.state.Load() == uint32(StateCreated) |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CREATED state check on line 738 is not atomic relative to line 737. After the CAS fails on line 737, another thread could transition the connection from CREATED to another state before line 738 checks it, leading to incorrect behavior. This should use a CAS for CREATED as well: cn.stateMachine.state.CompareAndSwap(uint32(StateCreated), uint32(StateCreated)) or combine into a single atomic operation.
| cn.stateMachine.state.Load() == uint32(StateCreated) | |
| cn.stateMachine.state.CompareAndSwap(uint32(StateCreated), uint32(StateCreated)) |
This PR fixes issues related to us having multiple different atomic flags in the pool connections for handoff, usability, etc. Since I tried to slightly changed the get and set operations of the pool, this correctness (which negatively impacted the performance in the first place) does not have real world drawbacks and improves developer experience.