-
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
Bug: Subscriber client crash when listen to '__redis__:invalidate' messages queue. #2117
Comments
Can I assume this pairs with #1461 (comment)? Presumably, then, client-side caching is breaking all the rules expected of pub/sub, which has historically always been a single value (per publish), which is what the code expects. If client-side caching internally broadcasts a multi-bulk, I'd say this isn't a "bug", it is a "feature request" - and a non-trivial one. We can't just hook that into the existing API, unless we just say "meh, well just take the first value" (or whatever gives the key). To support this properly requires a bespoke API focused on client-side cache support. Which would be a great thing to add, and we've been planning to add it: but it isn't necessarily trivial, as shown here. |
Thanks for the quick replay .. |
It isn't a question of leaving it without treatment; it is a question of where the time comes from. Everything takes time and effort. Note: I will readily accept, as a bug, the connection crash; we should fix that. However, the immediate "fix" for that is likely to be "silently ignore anything that isn't a simple-string or bulk-string", so: if this API is broadcasting a multi-bulk, it won't actually allow you to listen to the events. |
By the way, listen to keyspace channel work fine.. what is the different? |
presumably these system notifications are - uniquely - broadcasting a multi-bulk rather than a simple-string/bulk-string; indeed, if we look at this doc, we see the response: *3
$7
message
$20
__redis__:invalidate
*1
$3
foo the important bit is the last 3 lines,
It isn't as simple as that; if we did that, we'd be very brittle in terms of your code understanding where values start and end; if it is always length 1: this is a non-problem. Suggestion: for now, we check:
|
* fix #2117 for pub/sub, treak length-one arrays comparably to simple values * update release notes * actually check whether we're meant to be allowing arrays
Tnks , its work ! |
@GT185076 just to be sure I understand your comment: is that "that works for me; sounds like a plan!", or is it "I've built that locally / downloaded the myget, and can confirm that it now works!" ? |
What do you mean by "listen to keyspace channel"? Can you post a working sample code. Thx |
Presumably this means: the other inbuilt system broadcasts for keyspace notifications work successfully, because they push bulk-strings, not multi-bulks. |
I downloaded from myget and test it , I also clone the repository and look on the code . |
Correct. |
Here (need to remove spaces in the channel name) : `ConnectionMultiplexer conn = ConnectionMultiplexer.Connect("localhost"); switch (value) |
Found in :
https://github.com/StackExchange/StackExchange.Redis/issues/1461
Redis ver 6.2.6 (on docker's container)
Stackexchange ver : 2.2.79 / 2.5.61 ( on dotnet core 3.1 )
Lets say we have a subscriber for invalidate keys queue : (to implement near cache)
var subscriber = invalidator.GetSubscriber();
subscriber.Subscribe(" _ _ redis _ _ :invalidate").OnMessage(message =>
{
Console.WriteLine($"redis:invalidate: {message}");
});
We never get messages came from values changes or expired (we do get messages came from explicit publish call.)
When looking deeper we can see that the client id of the subscriber change after setting new values of tracking keys.. meaning it's was crash..
I got this : System.InvalidCastException: 'Cannot convert to RedisValue: MultiBulk' .
in this method :
Internal RedisValue AsRedisValue()
The text was updated successfully, but these errors were encountered: