fix(redis): pin dedicated connection in Conn for connection-stateful commands - #4864
Open
yangfei1024 wants to merge 3 commits into
Open
fix(redis): pin dedicated connection in Conn for connection-stateful commands#4864yangfei1024 wants to merge 3 commits into
yangfei1024 wants to merge 3 commits into
Conversation
…commands The redis adapter's Conn() returned a thin wrapper that only forwarded each command to the shared client, so every command borrowed and returned a pooled connection independently. For connection-stateful commands like MULTI/EXEC this breaks transaction semantics: the connection returns to the pool still in MULTI state, and concurrent requests that borrow it receive QUEUED replies instead of command results (e.g. an auth GET reading the literal string "QUEUED"). EXEC may also land on a different clean connection and fail with "EXEC without MULTI", leaving the polluted connection stuck in the pool until idle eviction. Now Conn() pins a dedicated underlying connection via go-redis StickyConnPool for non-cluster clients, so all commands issued through the same Conn run on the same physical connection, and Close() returns it to the pool. Cluster clients are not supported by go-redis Conn() and keep the previous shared-pool behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Some Redis commands are connection-stateful: they change the state of the
connection itself on the server side, and all subsequent commands on that same
connection are then interpreted in that state. The most common examples are
MULTI/EXEC(a transaction opened withMULTIbelongs to the connection —every command sent on it is queued and answered with
+QUEUEDuntilEXECorDISCARDarrives), plusWATCH,SELECT, etc.For such commands, a client must guarantee they all run on the same physical
connection. Native go-redis does this:
client.Conn()returns a connectionbacked by a sticky pool that holds one dedicated connection until
Close.The
redisadapter'sConn()does not. It returns a thin wrapper that forwardsevery command to the shared
client, so eachDoindependently borrows someconnection from the pool and returns it immediately (
connPool.Get→ execute →releaseConn). The consequences for connection-stateful commands:conn.Do(ctx, "MULTI")runs on some pooled connection A, and A goes back tothe pool while still in MULTI state on the server side.
transaction. It receives the literal string
"QUEUED"instead of the realreply — a
GETreturns"QUEUED", a TTL check returns"QUEUED". Whateverthe application does with that "value" is now wrong.
EXECmay be routed to a different, clean connection andfail with
EXEC without MULTI, while A stays stuck in MULTI state in thepool, poisoning every request that borrows it — potentially for a long time
(default
ConnMaxIdleTimeis 30 minutes).In short: code written as
conn, _ := g.Redis().Conn(ctx); conn.Do("MULTI"); …; conn.Do("EXEC")looks correct, matches the documented purpose ofConn("a connection object for continuous operations … call Close function manually
if you do not use this connection"), and works under native go-redis — but with
this adapter the transaction is silently broken and other innocent requests get
corrupted replies. It is a leaky abstraction: the API copies the shape of
go-redis
Connbut not its semantics.问题描述
Redis 有一部分命令是连接态命令:它们改变的是连接本身在服务端的状态,
之后发往同一条连接的所有命令都会在这个状态下被解释。最常见的例子是
MULTI/EXEC(MULTI开启的事务属于这条连接——其后这条连接上的每条命令都被排队、回复
+QUEUED,直到收到EXEC或DISCARD),此外还有WATCH、SELECT等。对这类命令,客户端必须保证它们全部落在同一条物理连接上。原生 go-redis
正是这么做的:
client.Conn()返回的连接由 sticky pool 支撑,独占一条物理连接直到
Close。而
redis适配层的Conn()没有做到。它返回的只是一个薄包装,把每条命令转发给共享的
client,因此每次Do都会独立地从池里借出某条连接、执行完立即归还(
connPool.Get→ 执行 →releaseConn)。对连接态命令的后果是:conn.Do(ctx, "MULTI")在某条池化连接 A 上执行,A 归还回池时在服务端仍处于 MULTI 状态。
真实回复而是字面量字符串
"QUEUED"——GET返回"QUEUED",TTL 查询返回"QUEUED"。应用程序拿这个"值"做的任何处理都随之出错。EXEC还可能被路由到另一条干净连接上,以EXEC without MULTI报错;而 A 带着 MULTI 状态滞留在池中,持续污染每个借到它的请求——持续时间可能很长(默认
ConnMaxIdleTime为 30 分钟)。简言之:
conn, _ := g.Redis().Conn(ctx); conn.Do("MULTI"); …; conn.Do("EXEC")这样的代码看起来正确、符合
Conn的文档定位("a connection object forcontinuous operations … call Close function manually if you do not use this
connection")、在原生 go-redis 下也确实正确——但在这层适配器下事务被静默破坏,
其他无关请求收到被污染的回复。这是一个典型的泄漏抽象:API 复制了 go-redis
Conn的形状,却没有复制它的语义。Reproduce
复现方式
Real-world example & timeline
A typical scenario where this bites — no exotic setup required:
Conn+MULTI … SADD … EXEC, because it looks like the idiomatic way to do atransactional batch write.
g.Redis().Do(ctx, "GET", key)on the same default group.They share one connection pool. The interleaving looks like this:
Two properties make this bug particularly nasty in production:
connection back to the transaction user, so everything appears to work. It
only breaks when another request happens to borrow the connection inside the
millisecond-scale MULTI→EXEC window.
EXECdoes land on A, the queuedcommands of the victims are submitted together with the transaction and A
becomes clean again, leaving no obvious trace. Only the unlucky branch
(
EXEC without MULTI) leaves a poisoned connection behind.Typical symptoms reported by application logs: an unrelated key's value is
occasionally the 6-character string
"QUEUED"; intermittentEXEC without MULTIerrors from the transaction user; hard to reproduce becauseeach individual piece looks correct.
真实案例与时序图
一个很容易踩中的典型场景,不需要任何特殊配置:
Conn+MULTI … SADD … EXEC批量写一个 set——这看起来正是事务批量写入的地道写法。
g.Redis().Do(ctx, "GET", key)读一个毫不相关的 key。两者共享同一个连接池。交错时序如下:
两个特性使这个 bug 在生产环境中尤其难缠:
正常。只有当别的请求恰好在这个毫秒级的 MULTI→EXEC 窗口内借走该连接时才会
出问题。
EXEC恰好落回 A 上,受害请求被排队的命令会随事务一起提交、A 恢复干净,不留明显痕迹。只有不走运的分支(
EXEC without MULTI)才会留下一条被毒化的连接。应用日志里的典型症状:某个无关 key 的值偶发变成 6 个字符的字符串
"QUEUED";事务使用者偶发EXEC without MULTI报错;每段代码单独看都正确,因此极难复现定位。
Solution
Redis.Conn()now pins a dedicated underlying connection for non-clusterclients via go-redis
client.Conn()(backed by aStickyConnPool), so allcommands issued through the same
Connrun on the same physical connection,and
Conn.Closeactually returns it to the shared pool.connection).
Conn(); they keep the previousshared-pool behavior (
Conn.conn == nilfallback).Note:
Redis.Dointernally usesConn()+Close()per call, which nowborrows/returns the pinned connection once per command — the same pool Get/Put
cost as before, so no extra overhead for stateless usage.
修复方案
Redis.Conn()对非集群客户端,通过 go-redis 的client.Conn()(底层为StickyConnPool)钉住一条专用物理连接,使同一个Conn上发出的所有命令都落在同一条物理连接上,
Conn.Close真正把连接归还共享池。Conn()不支持集群客户端,集群保持原有共享池行为(
Conn.conn == nil时回退)。说明:
Redis.Do内部每次调用都会走一遍Conn()+Close(),现在相当于每条命令借还一次钉住连接——池的 Get/Put 开销与之前相同,无状态用法没有额外损耗。
Example (after the fix)
示例(修复后)
Tests
Added
TestConn_Transactioninredis_z_unit_conn_test.go, covering:affected by the MULTI state (previously they could receive
QUEUED);EXECreturns the correct transaction results;EXEC.Verified locally by actually running
go test -count=1 ./...incontrib/nosql/redis(Go 1.26.4 / Redis 8.10.1): all tests pass, including thenew
TestConn_Transaction. The only failures are the sentinel tests, whichrequire a local Sentinel environment and fail identically on master
(connection refused, unrelated to this change).
测试
在
redis_z_unit_conn_test.go中新增TestConn_Transaction,覆盖:QUEUED);EXEC返回正确的事务结果;EXEC之后钉住的连接是干净的。已在本地实际执行
go test -count=1 ./...(Go 1.26.4 / Redis 8.10.1)验证:contrib/nosql/redis全部通过,包括新增的TestConn_Transaction。唯一的失败项是 sentinel 相关测试,需要本地 Sentinel 环境,在 master 上同样失败
(连接被拒,与本次改动无关)。