Repository navigation
fix(ring): respect context cancellation when ring buffer is full - #1034
AruneshDwivedi wants to merge 1 commit into
Conversation
PutOne/PutMulti now propagate ctx to waitContextOrSignal so callers get their timeout/cancel promptly instead of blocking until a reader drains the ring. Fixes redis#960 — requests with a 10ms deadline previously stalled for seconds when the internal ring buffer saturated. Signed-off-by: Arunesh Dwivedi <arunesh.devops@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
Reviewed by Cursor Bugbot for commit 91201fe. Configure here.
| c1.Broadcast() | ||
| case <-done: | ||
| } | ||
| c1.L.Unlock() |
There was a problem hiding this comment.
Wait helper deadlocks the ring
High Severity
waitContextOrSignal takes c1.L and holds it for the whole select on ctx.Done(). NextWriteCmd, WaitForWrite, and NextResultCh need that same per-slot mutex, so once a full-ring PutOne/PutMulti parks, the connection write/read loops cannot finish the occupying command. done is closed only after Wait returns, so with context.Background() this is a permanent deadlock; with a deadline the pipeline is frozen until cancel.
Reviewed by Cursor Bugbot for commit 91201fe. Configure here.
| if ctxErr := waitContextOrSignal(ctx, n.c1, n.c2); ctxErr != nil { | ||
| atomic.AddUint32(&r.write, ^uint32(0)) // decrement write counter on cancel | ||
| n.c1.L.Unlock() | ||
| return nil, ctxErr |
There was a problem hiding this comment.
Cancel rolls back write unsafely
High Severity
On cancel, PutOne and PutMulti unconditionally AddUint32 -1 to r.write after already claiming a ticket with AddUint32(+1). write is a monotonic slot ticket, not a occupancy count. If another putter has taken a later ticket, the decrement leaves a hole in the sequence and can assign the same index to two waiters. read1/read2 still walk in order, so the consumer waits forever on the skipped slot.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 91201fe. Configure here.
| close(done) | ||
| if ctx.Err() != nil { | ||
| return ctx.Err() | ||
| } |
There was a problem hiding this comment.
Cancel wins over a freed slot
Medium Severity
After c1.Wait() returns, waitContextOrSignal returns ctx.Err() without the caller re-checking n.mark. A wakeup from FinishResult can free the slot at the same time the context is cancelled; the putter then aborts and rolls back write instead of claiming mark == 0. FinishResult uses Signal, so that wakeup is consumed and any remaining waiter on the same c1 may never run.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 91201fe. Configure here.
When the internal ring buffer is full, PutOne/PutMulti currently block indefinitely waiting for a reader to drain a slot. This means callers with a context deadline (e.g. 10ms timeout) can stall for seconds, completely ignoring their deadline.
This PR adds a context-aware wait helper (waitContextOrSignal) that listens for either a normal signal or context cancellation. When cancelled, the write counter is rolled back so the next non-cancelled put can still find a slot.
Includes tests verifying:
Note
Medium Risk
Changes concurrency behavior on the hot enqueue path; incorrect lock/cond handling could cause deadlocks or slot leaks, though scope is limited to full-buffer waits.
Overview
Ring buffer puts now honor
contextdeadlines when all slots are occupied. Instead of blocking forever onsync.Cond.Wait,PutOneandPutMultiuse a newwaitContextOrSignalhelper that wakes waiters on normal drain signals or onctxcancellation/timeout, returning the context error.On cancel, the PR rolls back the atomic write counter so a failed enqueue does not consume a ring slot. New tests cover prompt unblock on cancel/timeout, write-counter invariants, and the helper’s cancel path.
Reviewed by Cursor Bugbot for commit 91201fe. Bugbot is set up for automated code reviews on this repo. Configure here.