-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Pub/Sub fixes for subscribe/re-subscribe #1947
Conversation
We're working on pub/sub - breaking it out explicitly.
*Now* this should be stable killing and restoring both connections with proper PING routing in place.
This awaits the condition, rather than a magical delay previously.
This won't be fully accurate until #1947 fixed the PING routing, but getting a test fix into main ahead of time.
This won't be fully accurate until #1947 fixed the PING routing, but getting a test fix into main ahead of time.
Want to yank some of this into another PR ahead of time, getting files in.
…nd cleanup In prep for changes to how we handle subscriptions internally, this does several things: - Upgrades default Redis server assumption to 3.x - Routes PING on Subscription keepalives over the subscription bridge appropriately - Fixes cluster sharding from default(RedisKey) to shared logic for RedisChannel as well (both in byte[] form) - General code cleanup in the area (getting a lot of DEBUG/VERBOSE noise into isolated files)
@andre-ss6 Of course! I think there may be a confusing aspect of PublishAsync here though, in Redis that's how many subscribers were on that server, not on that cluster. So for example you can get a 0 back, but still get the subscription from the other node. It's going from Publisher- > Node A -> Node B -> Subscriber. This tripped me up in tests too, and is probably worth of a remark on the XML docs for this. |
@andre-ss6 Check out the |
@NickCraver Oh I see! Thanks a lot for the clarification! I was going to actually test against the callback, instead of the return of I will re-do the tests later then. Thanks again! |
@andre-ss6 Thanks for the poke here, I do appreciate the eyes :) I'm changing all the /// <returns>
/// The number of clients that received the message *on the destination server*,
/// note that this doesn't mean much in a cluster as clients can get the message through other nodes.
/// </returns> |
This is something that tripped me and at least 1 user up - let's save some pain.
…nge/StackExchange.Redis into craver/pub-sub-issues
This is effectively the behavior we had before (but we are ensured connected now). We just want the count here and to start them, so let's do that also pipelining the `n` commands as we did before instead of awaiting each. We also don't need the `Task` overhead. This makes moving to a variadic command set for this less urgent.
See |
Hey @NickCraver, thanks a lot for the help and for your time! I re-did the tests and they worked! However, not how I thought they would 😅, as they also worked on an old version of the library (1.2.6). Since it worked, however, I guess this is now a bit off-topic for the PR, so I sent you a message on gitter, if you have time to clarify some questions. But in any way, you already helped a lot. Thanks again! |
@@ -1,4 +1,4 @@ | |||
FROM redis:5 | |||
FROM redis:6.2.6 |
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.
+1; this gives us a few other unrelated things we can use
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.
this looks great and very well considered; unable to find anything other than pettiness, so: nice job sir
@mgravell If there's little things to tweak as well, happy to! (same for all the PRs), think we're about in a good state so more than happy to polish anywhere |
We're working on pub/sub - breaking it out explicitly from #1912. This relates to several issues and in general handling resubscriptions on reconnect.
Issues: #1110, #1586, #1830 #1835
There are a few things in play we're investigating:
PING
andGetBridge
)default(RedisKey)
)Overall this set of changes:
PendingSubscriptionState
(Subscription
has the needed bits to reconnect)RedisSubscriber
, rather thanSubscriber
,RedisSubscriber
, andConnectionMultiplexer
)RedisSubscriber
, we can cleanly useExecuteSync/Async
bits, get proper profiling, etc.Wait()
in sync paths)Subscription
object is added to theConnectionMultiplexer
tracking immediately, but the command itself actually goes to the server and back (unless FireAndForget) before returning for proper ordering like other commands.Task.Run()
loop - we now ensure reconnects as part of the handshakeinteractive
connected butsubscription
in an unknown state - both are connected before we fire the handler meaning the moment we come back from connect, subscriptions are in business.ConcurrentDictionary
since we only need limited locking around this and we only have it once per multiplexer.TrackSubscriptionsProcessor
- this was never setting the result but didn't notice in 8 years because downstream code never cared.Subscription
has a processor instance (with minimal state) because when the subscription command comes back then we need to decide if it successfully registered (if it didn't, we need to maintain it has no successful server)ConnectionMultiplexer
grew aDefaultSubscriber
for running some commands without lots of method duplication, e.g. ensuring servers are connected.GetHashSlot
onCommandChannelBase
with the newRedisChannel
-based methods so that operates correctlyNot directly related changes which helped here:
PubSub
tests that were unreliable before...but correctly so.TODO: I'd like to add a few more test scenarios here:
To consider:
SUBSCRIBE <key1> <key2>...
IsConnected
are slightly off in the not connected (CurrentServer is null
) case, because we'd say we're connected to where it would go even though that'd be non-deterministic without hashslot batching. I think this is really minor and shouldn't affect our decision.ConcurrentDictionary
vs. returning to locks around aDictionary