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.

Sign up for GitHub

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

Jump to bottom

Add asynchronous test for pubsub using RESP3 #1012

Merged
merged 2 commits into from
Dec 1, 2021
Merged

Add asynchronous test for pubsub using RESP3 #1012

merged 2 commits into from
Dec 1, 2021

Conversation

bjosv
Copy link
Contributor

@bjosv bjosv commented Oct 28, 2021 •
edited
Loading

This PR adds a testcase for pubsub using RESP3 and reuses the subscribe callback from the RESP2 testcase,
all to make sure we have a common behavior.
This requires a correction for unsubscribe responses which previously was given to the push callback.

Fixes #967

@bjosv bjosv marked this pull request as ready for review November 8, 2021 09:03
@michael-grunder
Copy link
Collaborator

Thanks, @bjosv I will go through this and get it merged this week.

@michael-grunder michael-grunder self-assigned this Nov 17, 2021
@bjosv
Include unsubscribe as a subscribe reply in RESP3
033e1aa 
By providing the (p)unsubscribe message via the subscribe callback,
instead of via the push callback, we get the same behavior in RESP3
as in RESP2.
@bjosv
Add asynchronous test for pubsub using RESP3
2ca4b7f 
The testcase will subscribe to a channel, and via a second client
a message is published to the channel. After receiving the message
the testcase will unsubscribe and disconnect.

This RESP3 testcase reuses the subscribe callback from the RESP2
testcase to make sure we have a common behavior.
@bjosv
Copy link
Contributor Author

bjosv commented Nov 19, 2021

Now rebased and updated to match the existing asynchronous testing on master.

@bjosv bjosv mentioned this pull request Nov 25, 2021
Merged
@bjosv
Copy link
Contributor Author

bjosv commented Nov 30, 2021

@michael-grunder Do you think handling the (p)unsubscribe in the subscribe-callback for RESP3 would be problematic due to backward compatibility?

@michael-grunder michael-grunder merged commit da5a4ff into redis:master Dec 1, 2021
@michael-grunder
Copy link
Collaborator

@bjosv I'll take a closer look, although I'm thinking v2.0.0 is probably good to release soon so we can fix this hack of the sds symbols we needed to do for Redis.

@bjosv bjosv deleted the async-pubsub-resp3-tests branch December 2, 2021 08:35
@zuiderkwast
Copy link
Contributor

zuiderkwast commented Dec 2, 2021 •
edited
Loading

@michael-grunder Why 2.0.0 and not just 1.1.0? Are there any backwards-incompatible changes needed to remove the sds hack?

There are as well differences in dict between Redis and Hiredis and probably more changes to come in Redis.

@michael-grunder
Copy link
Collaborator

@zuiderkwast We may be able to get away with v1.1.0 which would be fine with me. The issue, I suppose is that we are technically exporting the sds symbols so I don't think we can change their names (at least not by default) without a major version bump.

It's been a while since I looked at that branch though, we may be able to #ifndef our way around the conflict.

For v2.0.0 it's probably prudent to hide the internals of redisContext behind an opaque struct. Over the years this has caused many issues, especially for people new to C.

@zuiderkwast zuiderkwast mentioned this pull request Sep 1, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@michael-grunder michael-grunder

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Why does hiredis ignore my callback on UNSUBSCRIBE in RESP3?
3 participants
@bjosv @michael-grunder @zuiderkwast