fix: premature recycles in MGET/MSET helpers#980
Conversation
Signed-off-by: Rueian <rueiancsie@gmail.com>
🛡️ Jit Security Scan Results✅ No security findings were detected in this PR
Security scan by Jit
|
There was a problem hiding this comment.
Pull request overview
Fixes premature recycling of pooled command buffers used by cluster helper functions, to avoid unsafe reuse when DoMulti returns non-Redis errors (e.g., context cancel/deadline) where pipelining internals may still reference the command buffer.
Changes:
- Refactors
doMultiSetto accept the pooled*mgetcmdsbuffer and only return it tomgetcmdspwhen all results haveNonRedisError()==nil. - Updates
clusterMGetandclusterJsonMGetto conditionally return*mgetcmdsto the pool based on whether anyDoMultiresult has a non-Redis error. - Adds tests covering success paths, Redis-level errors, and non-Redis errors for the affected helper flows.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| helper.go | Adjusts pooling/recycling behavior for cluster helper command buffers to avoid premature reuse on non-Redis errors. |
| helper_test.go | Adds tests exercising cancellation/deadline and error cases for the updated recycling behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| defer func() { | ||
| if recycle { | ||
| mgetcmdsp.Put(cmds) | ||
| } | ||
| }() | ||
|
|
||
| for i, resp := range resps { | ||
| arr, err := resp.ToArray() | ||
| if err != nil { |
There was a problem hiding this comment.
In clusterMGet, if resp.ToArray() returns an error with no NonRedisError (e.g. unexpected reply type / Redis error), the function returns early before reaching the intl.PutCompletedForce(cmds.s[i]) loop. Since these commands are Pin()'d, skipping PutCompletedForce means their CommandSlice buffers won’t be returned to the cmds pool (reducing reuse) even though recycle is still true and it should be safe to recycle them. Consider deferring command recycling when recycle remains true so it also happens on early returns (but still avoid recycling on NonRedisError).
| defer func() { | ||
| if recycle { | ||
| mgetcmdsp.Put(cmds) | ||
| } | ||
| }() | ||
|
|
||
| for i, resp := range resps { | ||
| arr, err := resp.ToArray() | ||
| if err != nil { |
There was a problem hiding this comment.
Same pattern as clusterMGet: if resp.ToArray() errors without a NonRedisError in clusterJsonMGet, the function returns before the PutCompletedForce loop, so Pin()'d command buffers won’t be recycled even though recycle is true. Consider deferring PutCompletedForce for cmds.s under the same recycle condition so it runs on early returns too (while still skipping recycling on NonRedisError).
Signed-off-by: Rueian <rueiancsie@gmail.com>
f29dc4f to
549554d
Compare
Note
Medium Risk
Touches cluster multi-command helpers and command-buffer pooling behavior; incorrect recycling could cause data races or subtle request corruption under concurrency/timeouts.
Overview
Fixes premature recycling of pooled command buffers (
mgetcmdsp) used by cluster helpers.MSet/MDel/MSetNX/JsonMSetnow pass the pooled*mgetcmdsbuffer intodoMultiSet, which only returns the buffer to the pool when no response contains a non-Redis error (e.g., context cancellation/deadline).clusterMGetandclusterJsonMGetsimilarly stop deferringmgetcmdsp.Putand instead return buffers only after successful response processing.Adds
TestClusterHelpersMgetcmdspRecycleto cover recycling behavior across success, Redis-level errors, parse errors, and context-cancellation paths.Reviewed by Cursor Bugbot for commit 549554d. Bugbot is set up for automated code reviews on this repo. Configure here.