Skip to content
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

for pub/sub, treat length-one arrays comparably to simple values #2118

Merged
merged 3 commits into from
Apr 26, 2022

Conversation

mgravell
Copy link
Collaborator

fix #2117

it appears that some of the RESP2 fallback support for client-side caching uses multi-bulk payloads; this currently causes a connection break, so we definitely need to stop the failure, but we can also interpret unary arrays similarly to simple values, which should at least allow simple cases to work

for pub/sub, treak length-one arrays comparably to simple values
@mgravell mgravell changed the title for pub/sub, treak length-one arrays comparably to simple values for pub/sub, treat length-one arrays comparably to simple values Apr 26, 2022
Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Looks good, but if we can not recurse I think it'd be a bit simpler. Probably a reason I'm not seeing through 👍

parsed = value.AsRedisValue();
return true;
case ResultType.MultiBulk when value.ItemsCount == 1:
return TryGetPubSubPayload(in value[0], out parsed, allowArraySingleton: false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I think this would be simpler to take the first element as a RedisValue instead of recursion just for ease of understanding/maintenance, but there may be a future case of nested arrays I'm unaware of.

Copy link
Collaborator Author

@mgravell mgravell Apr 26, 2022

Choose a reason for hiding this comment

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

@NickCraver yah, avoiding edge cases and avoiding having to repeat the "what combinations are allowed" (the switch); ultimately the array here was unexpected, so... yeah: I don't want to trust anything

@mgravell mgravell merged commit 77a159c into main Apr 26, 2022
@mgravell mgravell deleted the fix-2117 branch April 26, 2022 10:47
@GT185076
Copy link

Well done , it's work !
Big question left : Is there a case with multiple values (more than one)..?

@mgravell
Copy link
Collaborator Author

Great question. I honestly don't know. If there is, the library will now silently ignore them (since it doesn't know what to do with them) rather than outright dying, which is... better.

@baytekink
Copy link

Well done , it's work ! Big question left : Is there a case with multiple values (more than one)..?

Can you please share the dlls or nuget package?

@mgravell
Copy link
Collaborator Author

The myget feed is here: https://www.myget.org/F/stackoverflow/api/v3/index.json - we'll push it to nuget when we're ready

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.

Bug: Subscriber client crash when listen to '__redis__:invalidate' messages queue.
4 participants