fix(pubsub): avoid panics on Channel conflict and Ring empty subscribe - #3968
fix(pubsub): avoid panics on Channel conflict and Ring empty subscribe#3968VedantMadane wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 924803beea
ℹ️ 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 c.stickyErr != nil { | ||
| return nil, c.stickyErr | ||
| } | ||
| if c.closed { | ||
| return nil, pool.ErrClosed |
There was a problem hiding this comment.
Let Close override the sticky error
When Ring.Subscribe, PSubscribe, or SSubscribe returns a failed PubSub and the caller uses either channel API, the receiver goroutine repeatedly gets stickyErr. After Close sets closed, this ordering still returns the sticky error instead of pool.ErrClosed, so initMsgChan/initAllChan never exits or closes the documented output channel; consumers can remain blocked and one goroutine leaks per failed subscription. Check closed before stickyErr, or otherwise make channels terminate for a permanently failed PubSub.
Useful? React with 👍 / 👎.
| internal.Logger.Printf(c.getContext(), | ||
| "redis: Channel can't be called after ChannelWithSubscriptions") |
There was a problem hiding this comment.
Avoid reading the receiver command from the logging path
When the caller invokes the conflicting channel API after the first channel API returns, that first call has already started a receiver goroutine which initializes c.cmd in ReceiveTimeout; this new c.getContext() call reads the same field without synchronization and can therefore race with that initialization even though the two public calls were sequential. The symmetric logging path in ChannelWithSubscriptions has the same problem; log with a context that does not inspect receiver-owned state or synchronize access to that state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 924803b. Configure here.
| func (c *PubSub) conn(ctx context.Context, newChannels []string) (*pool.Conn, error) { | ||
| if c.stickyErr != nil { | ||
| return nil, c.stickyErr | ||
| } |
There was a problem hiding this comment.
Sticky error blocks Channel close
High Severity
conn returns stickyErr before checking closed, so after Close on a failed PubSub (from Ring empty subscribe or shard lookup failure), Receive never yields pool.ErrClosed. initMsgChan / initAllChan only exit on that error, so calling Channel then Close leaves the receive goroutine running forever and for range on the channel hangs. That breaks the documented contract that the Go channel closes with the PubSub. The pool sticky pattern in SingleConnPool.Close overrides sticky with ErrClosed instead.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 924803b. Configure here.


Summary
Fixes #3761.
PubSub.Channel/ChannelWithSubscriptionsmutual-exclusion misuse no longer panics; returns a closed channel and logs a warning.Ring.Subscribe/PSubscribe/SSubscribeno longer panic on empty channels or shard lookup failure; return a PubSub with a sticky error (as the existing TODO suggested).Test plan
TestRingSubscribeEmptyChannelsNoPanicTestPubSubChannelMutualExclusionNoPanicNote
Medium Risk
Changes error handling in PubSub and Ring subscribe paths; callers that relied on panics may see errors or empty channels instead, but behavior is more predictable for production code.
Overview
Fixes #3761 by replacing several PubSub panics with recoverable behavior.
Ring
Subscribe/PSubscribe/SSubscribeno longer panic when channels are missing or shard routing fails. They return a normal*PubSubwith a sticky error (failedPubSub);Receiveand connection setup surface that error viaconn().Calling
ChannelafterChannelWithSubscriptions(or the reverse) no longer panics. The second call logs a warning and returns an already-closed Go channel sorange/selectcan fail safely.Tests cover empty Ring subscribe and the Channel API conflict.
Reviewed by Cursor Bugbot for commit 924803b. Bugbot is set up for automated code reviews on this repo. Configure here.