fix(conn): conn to have state machine #3559

ndyakov · 2025-10-24T10:10:31Z

No description provided.

…ne-conn

Copilot

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 ConnStateMachine with 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.}

redis.go

internal/pool/conn.go

internal/pool/conn_state.go

internal/pool/conn.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

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.

maintnotifications/errors.go

internal/pool/conn.go

internal/pool/conn_state.go

internal/pool/pool.go

internal/pool/conn_state_test.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* perf(pool): replace hookManager RWMutex with atomic.Pointer and add predefined state slices - Replace hookManager RWMutex with atomic.Pointer for lock-free reads in hot paths - Add predefined state slices to avoid allocations (validFromInUse, validFromCreatedOrIdle, etc.) - Add Clone() method to PoolHookManager for atomic updates - Update AddPoolHook/RemovePoolHook to use copy-on-write pattern - Update all hookManager access points to use atomic Load() Performance improvements: - Eliminates RWMutex contention in Get/Put/Remove hot paths - Reduces allocations by reusing predefined state slices - Lock-free reads allow better CPU cache utilization * perf(pool): eliminate mutex overhead in state machine hot path The state machine was calling notifyWaiters() on EVERY Get/Put operation, which acquired a mutex even when no waiters were present (the common case). Fix: Use atomic waiterCount to check for waiters BEFORE acquiring mutex. This eliminates mutex contention in the hot path (Get/Put operations). Implementation: - Added atomic.Int32 waiterCount field to ConnStateMachine - Increment when adding waiter, decrement when removing - Check waiterCount atomically before acquiring mutex in notifyWaiters() Performance impact: - Before: mutex lock/unlock on every Get/Put (even with no waiters) - After: lock-free atomic check, only acquire mutex if waiters exist - Expected improvement: ~30-50% for Get/Put operations * perf(pool): use predefined state slices to eliminate allocations in hot path The pool was creating new slice literals on EVERY Get/Put operation: - popIdle(): []ConnState{StateCreated, StateIdle} - putConn(): []ConnState{StateInUse} - CompareAndSwapUsed(): []ConnState{StateIdle} and []ConnState{StateInUse} - MarkUnusableForHandoff(): []ConnState{StateInUse, StateIdle, StateCreated} These allocations were happening millions of times per second in the hot path. Fix: Use predefined global slices defined in conn_state.go: - validFromInUse - validFromCreatedOrIdle - validFromCreatedInUseOrIdle Performance impact: - Before: 4 slice allocations per Get/Put cycle - After: 0 allocations (use predefined slices) - Expected improvement: ~30-40% reduction in allocations and GC pressure * perf(pool): optimize TryTransition to reduce atomic operations Further optimize the hot path by: 1. Remove redundant GetState() call in the loop 2. Only check waiterCount after successful CAS (not before loop) 3. Inline the waiterCount check to avoid notifyWaiters() call overhead This reduces atomic operations from 4-5 per Get/Put to 2-3: - Before: GetState() + CAS + waiterCount.Load() + notifyWaiters mutex check - After: CAS + waiterCount.Load() (only if CAS succeeds) Performance impact: - Eliminates 1-2 atomic operations per Get/Put - Expected improvement: ~10-15% for Get/Put operations * perf(pool): add fast path for Get/Put to match master performance Introduced TryTransitionFast() for the hot path (Get/Put operations): - Single CAS operation (same as master's atomic bool) - No waiter notification overhead - No loop through valid states - No error allocation Hot path flow: 1. popIdle(): Try IDLE → IN_USE (fast), fallback to CREATED → IN_USE 2. putConn(): Try IN_USE → IDLE (fast) This matches master's performance while preserving state machine for: - Background operations (handoff/reauth use UNUSABLE state) - State validation (TryTransition still available) - Waiter notification (AwaitAndTransition for blocking) Performance comparison per Get/Put cycle: - Master: 2 atomic CAS operations - State machine (before): 5 atomic operations (2.5x slower) - State machine (after): 2 atomic CAS operations (same as master!) Expected improvement: Restore to baseline ~11,373 ops/sec * combine cas * fix linter * try faster approach * fast semaphore * better inlining for hot path * fix linter issues * use new semaphore in auth as well * linter should be happy now * add comments * Update internal/pool/conn_state.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * address comment * slight reordering * try to cache time if for non-critical calculation * fix wrong benchmark * add concurrent test * fix benchmark report * add additional expect to check output * comment and variable rename --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

- Handle unexpected conn state changes

…ne-conn

ndyakov added 4 commits October 23, 2025 14:05

wip

27591cd

wip, used and unusable states

606264e

Merge remote-tracking branch 'origin/master' into ndyakov/state-machi…

0a75466

…ne-conn

polish state machine

5721512

ndyakov requested a review from htemelski-redis October 24, 2025 10:10

ndyakov mentioned this pull request Oct 24, 2025

fix(init): conn init should be thread safe #3555

Closed

ndyakov added 4 commits October 24, 2025 14:13

correct handling OnPut

663a60e

better errors for tests, hook should work now

7526e67

fix linter

92433e6

improve reauth state management. fix tests

21bd243

ndyakov requested a review from Copilot October 24, 2025 12:07

Copilot AI reviewed Oct 24, 2025

View reviewed changes

ndyakov and others added 11 commits October 24, 2025 15:20

Update internal/pool/conn.go

3f29463

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/pool/conn.go

de2f8ba

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

better timeouts

94fa920

empty endpoint handoff case

cfcf37d

fix handoff state when queued for handoff

3a53e1b

try to detect the deadlock

c4ed467

try to detect the deadlock x2

9ad6288

delete should be called

03b0003

improve tests

84e856e

fix mark on uninitialized connection

a2c7a25

Merge branch 'master' into ndyakov/state-machine-conn

23d0e0f

ndyakov requested a review from Copilot October 25, 2025 00:33

Copilot AI reviewed Oct 25, 2025

View reviewed changes

ndyakov and others added 5 commits October 25, 2025 21:44

Update internal/pool/conn_state_test.go

ffbe1e5

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/pool/conn_state_test.go

65a6ece

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/pool/pool.go

0964dcc

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/pool/conn_state.go

bc42307

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/pool/conn.go

33696fb

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

ndyakov added 2 commits October 25, 2025 21:53

fix error from copilot

13a4b3f

address copilot comment

07e665f

ndyakov mentioned this pull request Oct 25, 2025

fix(pool): pool performance #3565

Merged

ndyakov marked this pull request as ready for review October 27, 2025 13:06

ndyakov requested a review from ofekshenawa October 27, 2025 13:07

ndyakov added the maintenance label Oct 27, 2025

ndyakov and others added 6 commits October 28, 2025 00:49

initConn sets IDLE state

9448059

- Handle unexpected conn state changes

Merge remote-tracking branch 'origin/master' into ndyakov/state-machi…

b862bf5

…ne-conn

fix precision of time cache and usedAt

d5db534

allow e2e tests to run longer

dcd8f9c

Merge branch 'master' into ndyakov/state-machine-conn

f1c8884

Fix broken initialization of idle connections

0752aec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(conn): conn to have state machine #3559

fix(conn): conn to have state machine #3559

ndyakov commented Oct 24, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

fix(conn): conn to have state machine #3559

Are you sure you want to change the base?

fix(conn): conn to have state machine #3559

Conversation

ndyakov commented Oct 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants