-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(pubsub): avoid panics on Channel conflict and Ring empty subscribe #3968
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,11 @@ type PubSub struct { | |
| closed bool | ||
| exit chan struct{} | ||
|
|
||
| // stickyErr is set when PubSub is constructed in a failed state (e.g. Ring | ||
| // shard lookup failure). Subsequent operations return this error instead of | ||
| // panicking at construction time. | ||
| stickyErr error | ||
|
|
||
| cmd *Cmd | ||
|
|
||
| chOnce sync.Once | ||
|
|
@@ -72,6 +77,9 @@ func (c *PubSub) connWithLock(ctx context.Context) (*pool.Conn, error) { | |
| } | ||
|
|
||
| func (c *PubSub) conn(ctx context.Context, newChannels []string) (*pool.Conn, error) { | ||
| if c.stickyErr != nil { | ||
| return nil, c.stickyErr | ||
| } | ||
| if c.closed { | ||
| return nil, pool.ErrClosed | ||
|
Comment on lines
+80
to
84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| } | ||
|
|
@@ -575,8 +583,13 @@ func (c *PubSub) Channel(opts ...ChannelOption) <-chan *Message { | |
| c.msgCh.initMsgChan() | ||
| }) | ||
| if c.msgCh == nil { | ||
| err := fmt.Errorf("redis: Channel can't be called after ChannelWithSubscriptions") | ||
| panic(err) | ||
| // Already using ChannelWithSubscriptions — return a closed channel | ||
| // instead of panicking so callers can recover (issue #3761). | ||
| internal.Logger.Printf(c.getContext(), | ||
| "redis: Channel can't be called after ChannelWithSubscriptions") | ||
|
Comment on lines
+588
to
+589
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 Useful? React with 👍 / 👎. |
||
| ch := make(chan *Message) | ||
| close(ch) | ||
| return ch | ||
| } | ||
| return c.msgCh.msgCh | ||
| } | ||
|
|
@@ -600,8 +613,13 @@ func (c *PubSub) ChannelWithSubscriptions(opts ...ChannelOption) <-chan interfac | |
| c.allCh.initAllChan() | ||
| }) | ||
| if c.allCh == nil { | ||
| err := fmt.Errorf("redis: ChannelWithSubscriptions can't be called after Channel") | ||
| panic(err) | ||
| // Already using Channel — return a closed channel instead of panicking | ||
| // so callers can recover (issue #3761). | ||
| internal.Logger.Printf(c.getContext(), | ||
| "redis: ChannelWithSubscriptions can't be called after Channel") | ||
| ch := make(chan interface{}) | ||
| close(ch) | ||
| return ch | ||
| } | ||
| return c.allCh.allCh | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package redis_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/redis/go-redis/v9" | ||
| ) | ||
|
|
||
| func TestRingSubscribeEmptyChannelsNoPanic(t *testing.T) { | ||
| ring := redis.NewRing(&redis.RingOptions{ | ||
| Addrs: map[string]string{"shard1": "localhost:6379"}, | ||
| }) | ||
| defer ring.Close() | ||
|
|
||
| pubsub := ring.Subscribe(context.Background()) | ||
| if pubsub == nil { | ||
| t.Fatal("expected non-nil PubSub") | ||
| } | ||
| // Receive should surface sticky error, not panic. | ||
| _, err := pubsub.Receive(context.Background()) | ||
| if err == nil { | ||
| t.Fatal("expected sticky error from empty Subscribe") | ||
| } | ||
| _ = pubsub.Close() | ||
| } | ||
|
|
||
| func TestPubSubChannelMutualExclusionNoPanic(t *testing.T) { | ||
| // Construct via a client that may not be reachable — Subscribe without | ||
| // channels just builds a PubSub handle. | ||
| client := redis.NewClient(&redis.Options{Addr: "127.0.0.1:1"}) | ||
| defer client.Close() | ||
| pubsub := client.Subscribe(context.Background()) | ||
| defer pubsub.Close() | ||
|
|
||
| _ = pubsub.ChannelWithSubscriptions() | ||
| ch := pubsub.Channel() | ||
| // Channel must return a closed/empty channel rather than panicking. | ||
| select { | ||
| case _, ok := <-ch: | ||
| if ok { | ||
| t.Fatal("expected closed channel from conflicting Channel() call") | ||
| } | ||
| default: | ||
| // non-blocking closed channel may still be receivable; try again with receive | ||
| _, ok := <-ch | ||
| if ok { | ||
| t.Fatal("expected closed channel") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sticky error blocks Channel close
High Severity
connreturnsstickyErrbefore checkingclosed, so afterCloseon a failedPubSub(fromRingempty subscribe or shard lookup failure),Receivenever yieldspool.ErrClosed.initMsgChan/initAllChanonly exit on that error, so callingChannelthenCloseleaves the receive goroutine running forever andfor rangeon the channel hangs. That breaks the documented contract that the Go channel closes with thePubSub. The pool sticky pattern inSingleConnPool.Closeoverrides sticky withErrClosedinstead.Additional Locations (2)
pubsub.go#L219-L235pubsub.go#L777-L787Reviewed by Cursor Bugbot for commit 924803b. Configure here.