Resp3 oob push support #841
Conversation
Adds the ability to specify a custom RESP3 push handler in both sync and async hiredis API. The async API automatically catches non-subscribe based PUSH messages even if the user doesn't set a handler, so they won't interfere with subscription logic (and so they will be freed with freeReplyObject). See redis#825
This commit reworks our RESP3 support to be less surprising to the user
and more backward compatible with existing usage of our redisOptions
struct.
For syncrhonous hiredis we now automatically set the RESP3 push handler
to freeReplyObject if one isn't specified by the user and if they
haven't set the REDIS_OPT_NO_PUSH_HANDLER flag.
This means that existing code like this, will work in unsurpising ways:
```c
/* This will automatically use freeReplyObject for any PUSH messages */
redisOptions options = {0};
redisContext *c = redisConnectWithOptions(&options);
```
If the user does not want a handler, that is possible too either by
setting our "no handler" flag, or by updating the callback to NULL after
we create the context:
```c
/* First method */
redisOptions options = {0};
options.options |= REDIS_OPT_NO_PUSH_HANDLER;
redisContext *c = redisConnectWithOptions(&options);
/* Second method */
redisOptions options = {0};
redisContext *c = redisConnectWithOptions(&options);
redisSetPushHandler(c, NULL);
```
In the async API we automatically set the REDIS_OPT_NO_PUSH_HANDLER flag
if the user doesn't specify a handler on construction. Spontaneous
PUSH messages will still be freed however so as not to leak memory.
See: redis#825
Get Redis 6 from Chris Lea's redis ppa and add some tests of the PUSH handler support. See redis#825
Properly clean up allocated memory. See: redis#825
|
The failing Windows test seems choco related. I'll see if I can get it working again by switching to memurai |
Switch to memurai on Windows to get unit tests working again. See: redis#825
Refactor how we can configure (or not configure) a PUSH handler to hopefully make the intentionality more clear. Additionally fix a bug where we weren't actually using the handler specified in redisOptions when constructing our hiredis context. See: redis#825
|
@yossigo If you're good with the API I will get it merged and add some documentation. Then I can put together v1.0.0 RC1 🎉 Edit: Actually I think I'm going to rework the callback to provide access to the context as well (which will give our users access to any |
|
@michael-grunder yes |
* Better conform to existing hiredis naming conventions
(e.g. redisPushHandler -> redisPushFn)
* Add context->privdata to the sync PUSH handler prototype so the user
has a simple mechanism to figure out which context is receiving the
PUSH reply.
* Rework the async PUSH handler prototype to be identical to our
existing redisCallbackFn:
void redisCallbackFn(redisAsyncContext *ac, void *void);
This should be really convenient for use cases where the user is using
a "generic" callback handler, because they don't need to use a
pass-thru function. Additionally it gives us a reserved argument for
some future purpose.
See: redis#825
|
@yossigo If you have some time I'd love a second pair of eyes on the (famous last words) final reworking of the API. I've bikeshed this feature to death but don't mind making changes since we're stuck with it once we officially release v1.0.0 TL;DR of the new API I reworked the callback prototypes: typedef void (redisPushFn)(void *, void *);
typedef void (redisAsyncPushFn)(struct redisAsyncContext *, void *, void *);As you can see I added c->privdata to the arguments I send to the sync callback and changed the async callback to match our existing If you think this would be confusing for users it's easy to drop the final argument, which is currently unused. |
There was a problem hiding this comment.
@michael-grunder Definitely not bikeshedding, I think coming up with a solid public API matters a lot. This looks good to me!
I don't have a very strong opinion on the prototype, I think both ways work. Having a consistent prototype is definitely a good thing, but we're no longer there anyway with the connect/disconnect/timer callbacks which could be consistent as well. I think it works both ways!
* Add documentation for our PUSH handlers as well as a small example program with annotations. * One final tweak of the callback prototype (remove unused argument) See: redis#825
Implement proper RESP3 push message support in both sync and async hiredis.
The change intercepts
RESP3PUSH messages by default in sync hiredis and callsfreeReplyObjectwhen they are received. If the user wishes to do something specific with the replies they can either set a handler up inredisOptionsor specify a handler after connected withredisSetPushHandlerThis works similarly in async hiredis except we always call
freeObjectregardless of whether the user has specified an additional handler (because async hiredis always frees replies).It also adds some tests (in Linux) for the handler by using Chris Lea's Redis PPA (so we can test against Redis 6.0.x).
See: #825
cc: @yossigo, @lukepalmer