Skip to content

Conversation

@dranikpg
Copy link
Contributor

@dranikpg dranikpg commented Jun 7, 2025

Fixes #5139

Dragonlfy had the following issue:

  • After any kind of subscribe is issued, the client is in pubsub mode. No commannds, except pubsub ones, can be executed
  • To exit the pubsub mode, the client has to unsubscribe from all channels. In that case the server replies with 0 as the number of pending subscriptions
  • Many clients, like io-redis, rely on this condition to exit pubsub mode
  • The reply is a regular resp3 push message over the same connection. The client can be stuck in pubsub for some time if it has a large socket buffer, large pile of previous messages buffered somewhere, etc... It only matters it's delivered eventually and delivered last
  • Dragonfly checks subscriber validity only at the point of fetching them from the channel store. After that, it waits on backpressure and dispatches to the clients native threads. During this time, they might have already unsubscribed. So for Dragonfly, reading a successful reply from "unsubscribe" doesn't guarantee no more messages will be delivered.
  • This PR fixes the "unsubscribe all" case. In general, we have the same problem when unsubscribing only from a specific channel.

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg dranikpg force-pushed the fix-pubsub-unsub branch from a8890a6 to 091ae2c Compare June 7, 2025 06:29
// Even after removing all subscriptions, we still can receive messages delayed
// by inter-thread dispatches or backpressure.
// TODO: filter messages from channels the client unsubscribed from
// if (self->cntx()->subscriptions == 0 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can apply the similar filter at the point where we publish right ?

Copy link
Contributor Author

@dranikpg dranikpg Jun 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand 🙂 At the time we publish we just fetch all subscribers and they're all valid at that instant. The problem is that in-between determining candidates for publishing and actually sending the message on a client's thread the client performs operations (like unsubbing)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I see. What I had in mind is to detect the presence of pending unsubscribe messages in the dispatch queue at the point we call SendPubMessageAsync. Thinking about it now it's unnecessarily complicated as the connection might be subscribed to multiple channels or the dispatch q might contain sequences of subscribe/unsubscribe messages making it difficult to detect if what we about to push to the dispatch queue is considered stale

@dranikpg dranikpg force-pushed the fix-pubsub-unsub branch from db6fffb to cea22a2 Compare June 7, 2025 08:55
@dranikpg dranikpg force-pushed the fix-pubsub-unsub branch from cea22a2 to 05813b4 Compare June 7, 2025 08:59
@dranikpg dranikpg requested a review from romange June 7, 2025 10:31
@dranikpg dranikpg merged commit 3ecbbe5 into main Jun 8, 2025
10 checks passed
@dranikpg dranikpg deleted the fix-pubsub-unsub branch June 8, 2025 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PUBSUB Messages Sent As Command Replies

4 participants