feat(pool): honor context cancellation for blocking reads (BRPop) - #3934
feat(pool): honor context cancellation for blocking reads (BRPop)#3934eltypical wants to merge 10 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d87b32ef8
ℹ️ 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".
| // Fast cancellation path: if the context is done, abort before any socket ops. | ||
| if ctx != nil { | ||
| if err := ctx.Err(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When the context is canceled after the read has already started, as in the added BRPop(ctx, 0, key) test, this preflight check has already passed and cmdTimeout supplies timeout == 0, which leaves the socket with no read deadline unless the context had a deadline. The goroutine then remains blocked in fn(cn.rd) until Redis replies or the connection closes, so cancel() still will not make an infinite blocking pop return context.Canceled; the read deadline/connection needs to be updated when ctx.Done() fires while the read is in progress.
Useful? React with 👍 / 👎.
| - name: Run tests with race detector | ||
| env: | ||
| REDIS_ADDR: localhost:6379 | ||
| run: go test -v -race ./... |
There was a problem hiding this comment.
This workflow starts only a single Redis service on 6379, but go help packages confirms the ... wildcard expands across package directories, so go test -v -race ./... includes the root package's Ginkgo suite. I checked main_test.go, where the suite connects to ring shards, sentinel nodes, and cluster nodes unless RE_CLUSTER is set, so this new PR workflow will fail before exercising the BRPop regression test. Use the repo's Docker Compose test stack/Makefile or restrict the command to a test target that only needs this one service.
AGENTS.md reference: AGENTS.md:L30-L39
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Hello @eltypical, I can see the CI is failing, would you be able to fix that so we can review this PR as soon as possible. If you need any assistance let me know. |
- WithReader: start a ctx watcher when no read deadline is set (timeout==0 and no ctx deadline) to SetReadDeadline(now) when ctx.Done() fires, unblocking the kernel Read without closing the socket. - Leaves hot path unchanged when a deadline already exists; zero overhead when ctx is nil. This addresses reviewer feedback that cancellation occurring during a blocking read must interrupt the read rather than waiting for a reply/close.
|
Update pushed:
Next: I’ll watch CI and address any remaining failures. If you prefer closing the connection on ctx.Done() instead of SetReadDeadline, let me know and I can switch to that approach. |
|
CI updates pushed:
Commits: 7cd76b0, 3798075, c459336. I’ll monitor the checks and follow up with any failing test triage. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit c459336. Configure here.
| if cancelWatchDone != nil { | ||
| close(cancelWatchDone) | ||
| } | ||
| return err |
There was a problem hiding this comment.
Context cancellation returns timeout error, not context.Canceled
High Severity
When the watcher goroutine fires on ctx.Done(), it sets an immediate read deadline via SetReadDeadline, which causes the blocked read to return a raw network timeout error (*net.OpError with Timeout() == true). WithReader returns this timeout error as-is without checking ctx.Err(). The caller receives a timeout error instead of context.Canceled, breaking error detection via errors.Is(err, context.Canceled). The included test asserts err != context.Canceled and will fail because the actual error is a net timeout.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c459336. Configure here.
|
|
||
| err := fn(cn.rd) | ||
| if cancelWatchDone != nil { | ||
| close(cancelWatchDone) |
There was a problem hiding this comment.
Watcher goroutine may set stale deadline after completion
Low Severity
After fn returns successfully and close(cancelWatchDone) is called, if the context was also cancelled around the same time, Go's select may non-deterministically pick c.Done() over done, causing SetReadDeadline(now) to execute on the connection after WithReader has already returned. This sets a stale past deadline on a connection that may be returned to the pool, potentially causing a spurious timeout on the next command using that connection before it sets its own deadline.
Reviewed by Cursor Bugbot for commit c459336. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c45933691c
ℹ️ 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".
| if cancelWatchDone != nil { | ||
| close(cancelWatchDone) | ||
| } | ||
| return err |
There was a problem hiding this comment.
When a context without a deadline is canceled while a blocking read is in progress, the watcher unblocks the socket by setting an immediate read deadline, so fn(cn.rd) returns the net timeout from the connection. Returning that error unchanged means callers of BRPop(ctx, 0, ...) see an i/o timeout rather than context.Canceled, so the added regression test still fails in the cancellation path it is meant to cover; check ctx.Err() after the forced-deadline read before returning.
Useful? React with 👍 / 👎.
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
| # Skip on fork PRs; run for pushes or same-repo PRs | ||
| if: ${{ github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) }} |
There was a problem hiding this comment.
This workflow still declares a weekly schedule trigger, but the new job-level condition only permits push and same-repo pull_request events. On the cron event github.event_name is schedule, so the only job is skipped and the weekly vulnerability scan silently stops running; include schedule in the condition or gate only the forked-PR case.
Useful? React with 👍 / 👎.
| // but we do have a context, spawn a watcher to force an immediate deadline | ||
| // when ctx.Done() fires. This unblocks an in-flight Read without closing | ||
| // the socket and without affecting the common hot path where a deadline exists. | ||
| if ctx != nil && dl.Equal(noDeadline) { |
There was a problem hiding this comment.
This predicate only starts the cancellation watcher when the computed socket deadline is empty. If a caller uses context.WithTimeout(..., time.Minute) for BRPop(ctx, 0, key) and then calls cancel() after the read has started, dl is the future context deadline, so no goroutine moves the read deadline forward and the command can remain blocked until Redis replies or that minute expires. Gate the watcher on ctx.Done() being non-nil and let it handle early cancellation even when an initial deadline already exists.
Useful? React with 👍 / 👎.
| case <-c.Done(): | ||
| _ = nc.SetReadDeadline(time.Unix(0, getCachedTimeNs())) |
There was a problem hiding this comment.
When the read finishes normally at about the same time the caller cancels the context, cancelWatchDone and ctx.Done() can both be closed before this goroutine runs, and Go's select may choose the cancellation case. That can set a past read deadline after WithReader has returned and the connection has been put back into the pool, so a subsequent command on the same socket can fail immediately with an artificial timeout; make the stop path win once the read is complete or otherwise reset/guard the deadline update.
Useful? React with 👍 / 👎.
Technical Summary
Ensures blocking socket reads (e.g.,
BRPopwith zero timeout) strictly honor caller context cancellation by inspectingctx.Err()and forcing an immediate connection read deadline upon context completion.Impact Matrix
redis/go-redisinternal/pool/conn.goBRPop(0)when context is canceledctx.Done(), zero goroutine leakReproduction & Proof
brpop_ctx_cancel_test.goprovingBRPopreturns promptly on context cancellation without hanging socket reads.go test -race.Root Cause Analysis
When
ContextTimeoutEnabledis false or when executing zero-timeout blocking commands, network socket reads bypassedctx.Done()checks, keeping the goroutine blocked in raw TCP read state until connection drop.Note
Medium Risk
Changes the hot-path read/deadline behavior in the connection pool and adds a per-blocking-read goroutine only when there is no read deadline; low blast radius for typical commands but affects all reads that use WithReader.
Overview
Blocking commands like
BRPopwith timeout 0 could keep a goroutine stuck on a socket read after the caller canceled the context, because reads with no read deadline never observedctx.Done().Conn.WithReadernow bails out immediately ifctx.Err()is already set, and when the computed read deadline is “none” but a context is present it starts a small watcher that sets an immediate read deadline onctxcancellation so an in-flight read unblocks without closing the connection.deadline()also returns an immediate deadline when the context is already canceled.A new
TestBRPopContextCancellationintegration test coversBRPopreturningcontext.Canceledpromptly.CI workflows skip heavy jobs on fork PRs, and Go matrix versions are pinned to 1.24.x / 1.25.x instead of
stable/oldstable(and govulncheck uses 1.24.x).Reviewed by Cursor Bugbot for commit c459336. Bugbot is set up for automated code reviews on this repo. Configure here.