Subscriber unsubscribes after subscribing on reconnect #1110
Comments
|
Any information on this? This makes the library unusable for us as the subscriptions are unreliable. |
|
I haven't had chance to look (all support here comes from our personal
time, etc), but I agree it looks incorrect. My suspicion is that it
*thinks* it is unsubscribing from the old primary - most likely what I need
to do here is track the endpoint currently subscribed, and make sure we
include that in all routing decisions.
…On Fri, 19 Apr 2019, 04:52 Stephen Hellicar, ***@***.***> wrote:
Any information on this?
I should add that the internal subscribers dictionary still thinks its
subscribed.
This makes the library unusable for us as the subscriptions are unreliable.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1110 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAEHMBCEZYCQSP5UHLG7ZLPRE6XBANCNFSM4HDO7VPA>
.
|
|
No problem, I appreciate any work done in peoples personal time. I would have thought it was an error with how the events were invoked/queued, as they are correct, just occur out of order. |
8 tasks
NickCraver
added a commit
that referenced
this issue
Feb 4, 2022
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: - [x] Subscription heartbeat not going over the subscription connection (due to `PING` and `GetBridge`) - [x] Subscriptions not reconnecting at all (or potentially doing to and unsubscribing according to some issues) - [x] Subscriptions always going to a single cluster node (due to `default(RedisKey)`) Overall this set of changes: - Completely restructures how RedisSubscriber works - No more `PendingSubscriptionState` (`Subscription` has the needed bits to reconnect) - Cleaner method topology (in `RedisSubscriber`, rather than `Subscriber`, `RedisSubscriber`, and `ConnectionMultiplexer`) - By placing these on `RedisSubscriber`, we can cleanly use `ExecuteSync/Async` bits, get proper profiling, etc. - Proper sync/async split (rather than `Wait()` in sync paths) - Changes how subscriptions work - The `Subscription` object is added to the `ConnectionMultiplexer` tracking immediately, but the command itself actually goes to the server and back (unless FireAndForget) before returning for proper ordering like other commands. - No more `Task.Run()` loop - we now ensure reconnects as part of the handshake - Subscriptions are marked as not having a server the moment a disconnect is fired - Question: Should we have a throttle around this for massive numbers of connections, or async it? - Changes how connecting works - The connection completion handler will now fire when the _second_ bridge/connection completes, this means we won't have `interactive` connected but `subscription` in an unknown state - both are connected before we fire the handler meaning the moment we come back from connect, subscriptions are in business. - Moves to a `ConcurrentDictionary` since we only need limited locking around this and we only have it once per multiplexer. - TODO: This needs eyes, we could shift it - implementation changed along the way where this isn't a critical detail - Fixes the `TrackSubscriptionsProcessor` - this was never setting the result but didn't notice in 8 years because downstream code never cared. - Note: each `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 a `DefaultSubscriber` for running some commands without lots of method duplication, e.g. ensuring servers are connected. - Overrides `GetHashSlot` on `CommandChannelBase` with the new `RedisChannel`-based methods so that operates correctly Not directly related changes which helped here: - Better profiler helpers for tests and profiler logging in them - Re-enables a few `PubSub` tests that were unreliable before...but correctly so. TODO: I'd like to add a few more test scenarios here: - [x] Simple Subscribe/Publish/await Until/check pattern to ensure back-to-back subscribe/publish works well - [x] Cluster connection failure and subscriptions moving to another node To consider: - [x] Subscription await loop from EnsureSubscriptionsAsync and connection impact on large reconnect situations - In a reconnect case, this is background and only the nodes affected have any latency...but still. - [ ] TODOs in code around variadic commands, e.g. re-subscribing with far fewer commands by using `SUBSCRIBE <key1> <key2>...` - In cluster, we'd have to batch per slot...or just go for the first available node - ...but if we go for the first available node, the semantics of `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. - [x] `ConcurrentDictionary` vs. returning to locks around a `Dictionary` - ...but if we have to lock on firing consumption of handlers anyway, concurrency overhead is probably a wash.
|
This is drastically improved in #1947 and will be in the 2.5 release - thank you for the detail! |
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Issue
Subscriber very occasionally unsubscribes after subscribing after the subscriber connection is restored from a hard disconnect (port being closed).
This seems to be related to issue: #273
But I can still reproduce this.
Version
Reproduced with the following versions.
1.2.6
2.0.519
2.0.588
Behaviour
using redis-monitor on the server, the following is observed.
Expected/Normal Behaviour
OR
The text was updated successfully, but these errors were encountered: