Skip to content

fix(pool): data race on Conn.onClose during init vs Close - #3966

Open
saddamr3e wants to merge 1 commit into
redis:masterfrom
saddamr3e:conn-onclose-race
Open

fix(pool): data race on Conn.onClose during init vs Close#3966
saddamr3e wants to merge 1 commit into
redis:masterfrom
saddamr3e:conn-onclose-race

Conversation

@saddamr3e

@saddamr3e saddamr3e commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

go test -race, internal/pool:

WARNING: DATA RACE
Read at 0x... by goroutine 61:
  internal/pool.(*Conn).Close()                 conn.go:970
Previous write at 0x... by goroutine 60:
  internal/pool.(*Conn).SetOnClose()            conn.go:638
  internal/pool.(*Conn).ExecuteInitConn()       conn.go:649
  internal/pool.(*Conn).SetNetConnAndInitConn() conn.go:704

initConn installs the close callback (the StreamingCredentialsProvider unsubscribe) via SetOnClose while the connection is INITIALIZING inside SetNetConnAndInitConn. Conn.Close transitions to CLOSED from any state, then reads and nils cn.onClose with no synchronization, so a pool shutdown or connection removal that closes a conn whose (re)init is still in flight races the setter.

Store the hook in atomic.Pointer[func() error] so the setter and Close no longer race, with no per-connection mutex so Conn stays slim; Swap(nil) in Close also runs the hook at most once under a concurrent double-close. Regression test drives SetNetConnAndInitConn against Close and is clean under -race.


Note

Low Risk
Localized synchronization fix in pool connection teardown; behavior unchanged aside from eliminating the race and ensuring at-most-once callback invocation under concurrent close.

Overview
Fixes a data race on Conn.onClose when connection init (SetNetConnAndInitConnSetOnClose, e.g. streaming-credentials unsubscribe) runs concurrently with Close during pool shutdown or removal.

onClose is now an atomic.Pointer[func() error]: SetOnClose stores the hook (or clears with nil), and Close uses Swap(nil) to read, clear, and invoke the callback once without a per-connection mutex. Comments document why atomics were chosen over a mutex on Conn.

Adds TestConnOnCloseRaceWithInitConn, which hammers concurrent init and close under the race detector.

Reviewed by Cursor Bugbot for commit 8e39e98. Bugbot is set up for automated code reviews on this repo. Configure here.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8e39e98346

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread internal/pool/conn.go
cn.onClose.Store(nil)
return
}
cn.onClose.Store(&fn)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Run callbacks installed after close

When Close wins the race and completes Swap(nil) before an in-flight initConn reaches this store, SetOnClose installs the streaming-provider unsubscribe callback on an already-closed connection, so no later Close will invoke it. This leaves the provider subscribed to a dead connection—the exact shutdown/reinitialization interleaving this change targets—while the new regression test checks only for a data-race report and never verifies callback delivery. Preserve a terminal closed sentinel in the atomic slot or otherwise synchronize installation with the CLOSED transition so a late callback runs immediately.

AGENTS.md reference: AGENTS.md:L172-L180

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant