Add finalizer support to async Redis client #926

tudor · 2021-02-19T21:50:14Z

Also add libevent example that tests it and also demonstrates pubsub.

Fixes #925

tudor · 2021-03-02T22:45:01Z

@michael-grunder have you had a chance to take a look at this? Thank you!

michael-grunder · 2021-03-10T02:52:55Z

Hi @tudor, sorry for the delay.

The logic looks good at first glance. We have to be pretty careful about backward compatibility now that we've released v1.0.0, but I will test the logic in detail over the next couple of days.

tudor · 2021-03-19T20:04:10Z

@michael-grunder I don't think there are any backwards compatibility issues. The only thing that changes in a potentially backwards incompatible way is struct redisCallback (its size and the offsets of the pendingSubs and privdata members), but that's not allocated explicitly by users, nor is it used in any inline functions (in fact, it probably shouldn't be in a public header file at all).

tudor · 2021-03-19T20:05:32Z

I guess code that explicitly accesses redisAsyncContext::replies would break, but I don't imagine that was ever intended for public consumption.

kristjanvalur · 2021-03-30T12:46:20Z

I was just about to open an issue asking for this yesterday. I presume this is to support knowing when hiredis forgets the callback.
A couple of questions:

The finalizer seems necessary only for subsriptions, because regular callbacks are called once, and only once, right?
There is code in the subscription logic that replaces subscription callbacks in the dicts, if a previous subscription exists. Are the finalizers called in that case? Should they? Only if the privdata member is different?

tudor · 2021-03-31T01:43:03Z

@kristjanvalur

I chose to run the finalizer for regular callbacks as well, because that allows me to generically free state in the C++ code that sits on top of hiredis without having to know whether the callback is a subscription callback or not. If you don't want a finalizer, pass NULL :)
Similarly, I chose to always run the finalizer in the replacement case. Your old callback will never be called again (finalize it), and the new one will.

kristjanvalur · 2021-03-31T08:25:50Z

async.c

@@ -782,6 +809,7 @@ static int __redisAsyncCommand(redisAsyncContext *ac, redisCallbackFn *fn, void
            if (de != NULL) {
                existcb = dictGetEntryVal(de);
                cb.pending_subs = existcb->pending_subs + 1;
+                __redisRunFinalizer(ac,existcb);


Wouldn't it be prudent to check here if the contents of extcb is different from cb? In particular the privdata member, but probably also the callback and finalizer, in case the caller uses dynamically created stubs and NULL for privdata.
A caller might re-register to the same calback, particularly in light of the rather vague docuementation of hiredis

Dynamically creating functions is hard, so if you want dynamically-allocated state, you'll have trampoline functions and keep all the state in privdata. So a check for privdata would probably suffice.

But I stand by the belief that, as far as hiredis is concerned, we should keep the interface simple: if you replace a callback, the old one is gone (and finalized), and the new one takes its place.

(Ideally, hiredis wouldn't replace the callback if you use multiple callbacks to subscribe to the same channel-- it would just call all of them! but I understand why they made the decision that they did, and having multiple callbacks called with the same redisReply* would mean that you can't mutate the redisReply any more and you have to treat it as const.)

I agree in principle, but async programming is hard, and as a precautionary principle, I would make sure that I didn't finalize the callback that I am installing. Checking that the finalize calllback and the privdata are different would make sense.
Alternatively, we could improve upon the barely existing documentation by stating that calling redisAsyncCall*() to subscribe, more than once, to the same pattern/channel was undefined.

Ideally, there should be separate api functions to subscribe/unsubscribe from channels/patterns because the calling semantics are completely different than for regular calls.

kristjanvalur · 2021-04-02T18:09:19Z

async.c

@@ -782,6 +809,7 @@ static int __redisAsyncCommand(redisAsyncContext *ac, redisCallbackFn *fn, void
            if (de != NULL) {
                existcb = dictGetEntryVal(de);
                cb.pending_subs = existcb->pending_subs + 1;
+                __redisRunFinalizer(ac,existcb);


I agree in principle, but async programming is hard, and as a precautionary principle, I would make sure that I didn't finalize the callback that I am installing. Checking that the finalize calllback and the privdata are different would make sense.
Alternatively, we could improve upon the barely existing documentation by stating that calling redisAsyncCall*() to subscribe, more than once, to the same pattern/channel was undefined.

Ideally, there should be separate api functions to subscribe/unsubscribe from channels/patterns because the calling semantics are completely different than for regular calls.

kristjanvalur · 2021-04-15T10:00:07Z

async.c

@@ -782,6 +809,7 @@ static int __redisAsyncCommand(redisAsyncContext *ac, redisCallbackFn *fn, void
            if (de != NULL) {
                existcb = dictGetEntryVal(de);
                cb.pending_subs = existcb->pending_subs + 1;
+                __redisRunFinalizer(ac,existcb);


if (existcb->privdata != cb.privdata) __redisRunFinalizer(ac, existcb);

…t tests it and also demonstrates pubsub

tudor · 2021-08-02T20:03:57Z

@kristjanvalur sorry for the long delay, I made the change you requested. PTAL?

kristjanvalur · 2021-08-03T09:23:08Z

LGTM, although honestly, not much is happening in this repo. I've got a bunch of PRs that have been stuck for months, including a bunch of defects/fixes.

tudor · 2021-08-03T15:57:57Z

@michael-grunder PTAL? C++ programmers worldwide would thank you :)

michael-grunder

If you don't mind making those small changes I will get this merged tomorrow morning (West coast, PDT).

michael-grunder · 2021-08-09T02:12:33Z

async.h

+    redisFinalizerCallback *finalizer;
    int pending_subs;
    void *privdata;


I think we have to put the new member at the end of the structure, as the current changes breaks ABI compatibility by inserting a member in the middle.

Suggested change

redisFinalizerCallback *finalizer;

int pending_subs;

void *privdata;

int pending_subs;

void *privdata;

redisFinalizerCallback *finalizer;

michael-grunder · 2021-08-09T02:36:26Z

async.c

@@ -502,11 +521,12 @@ static int redisIsSubscribeReply(redisReply *reply) {

 void redisProcessCallbacks(redisAsyncContext *ac) {
    redisContext *c = &(ac->c);
-    redisCallback cb = {NULL, NULL, 0, NULL};
+    redisCallback cb = {NULL, NULL, NULL, 0, NULL};


Since we're placing the new callback at the end.

Suggested change

redisCallback cb = {NULL, NULL, NULL, 0, NULL};

redisCallback cb = {NULL, NULL, 0, NULL, NULL};

redisCallback cb = {0}; would be fine too.

michael-grunder · 2021-08-09T02:43:20Z

examples/CMakeLists.txt

    TARGET_LINK_LIBRARIES(example-libevent hiredis event)
+    ADD_EXECUTABLE(example-libevent-pubsub example-libevent-pubsub.c)
+    TARGET_LINK_LIBRARIES(example-libevent-pubsub hiredis event)


We should also update the Makefile with this new example, although I'm happy to do that after we merge the PR.

michael-grunder · 2021-08-09T02:45:18Z

examples/example-libevent-pubsub.c

+}
+
+void getFinalizer(redisAsyncContext *c, void *privdata) {
+    printf("Get finalizer called\n");


Just to avoid the warning

Suggested change

printf("Get finalizer called\n");

(void)privdata;

printf("Get finalizer called\n");

michael-grunder · 2021-08-09T02:46:03Z

examples/example-libevent-pubsub.c

+}
+
+void subscribeFinalizer(redisAsyncContext *c, void *privdata) {
+    printf("Subscribe finalizer called\n");


Suggested change

printf("Subscribe finalizer called\n");

(void)privdata;

printf("Subscribe finalizer called\n");

zuiderkwast · 2023-03-08T17:04:19Z

@tudor are you still interested in finishing this?

tudor · 2023-03-09T17:15:18Z

@zuiderkwast No, I'm not planning to work on this any more. I ended up having to parse the request and response (to know whether it's pub/sub or regular command) anyway for a different reason, in which case I can tell when the callback may be called multiple times.

kristjanvalur · 2023-03-10T09:09:49Z

That's a shame, it looked like a worthwhile addition. If there is interest, I can pick this up.

zuiderkwast · 2023-03-10T09:30:02Z

@kristjanvalur I was going to pick it up myself, but you can do it if you want.

We want it for hiredis-cluster, so we can implement pubsub without actually looking at the commands sent by the user. We haven't fully decided if this is enough though.

kristjanvalur · 2023-03-10T09:31:49Z

@kristjanvalur I was going to pick it up myself, but you can do it if you want.

No, in that case it is best you do it, I haven't touched hiredis for more than two years and am quite rusty :)

zuiderkwast · 2023-03-10T09:53:06Z

Hehe, I'm not sure I'm less rusty with hiredis internals. :-) I was just going to rebase and apply Michael's comments. I'll invite you for review.

zuiderkwast · 2023-07-19T23:29:56Z

This PR doesn't correctly handle SUBSCRIBE with multiple channels with finalizer.

I've implemented this now in #1173. It's ready for review. Please have a look. (I'll try to get time to implement the RESET command too, and to allow calling commands while in monitor mode.)

I had to rewrite a lot, because a callback can be used for multiple channels, which can be usubscribed independenly. To keep track of the last reference to a callback (for calling the finalizer at the right time) I've added a reference counter in the callback struct.

michael-grunder self-requested a review February 20, 2021 21:51

michael-grunder self-assigned this Feb 20, 2021

kristjanvalur reviewed Mar 31, 2021

View reviewed changes

kristjanvalur suggested changes Apr 15, 2021

View reviewed changes

kristjanvalur reviewed Apr 15, 2021

View reviewed changes

tudor added 2 commits August 2, 2021 19:56

Add finalizer support to async Redis client; add libevent example tha…

dca7691

…t tests it and also demonstrates pubsub

Only call finalizer if privdata is different

6b40487

tudor force-pushed the finalizer branch from dc9e2fa to 6b40487 Compare August 2, 2021 20:03

michael-grunder requested changes Aug 9, 2021

View reviewed changes

zuiderkwast mentioned this pull request Mar 12, 2023

Add finalizer support in async API #1173

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add finalizer support to async Redis client #926

Add finalizer support to async Redis client #926

tudor commented Feb 19, 2021

tudor commented Mar 2, 2021

michael-grunder commented Mar 10, 2021

tudor commented Mar 19, 2021

tudor commented Mar 19, 2021

kristjanvalur commented Mar 30, 2021

tudor commented Mar 31, 2021

kristjanvalur Mar 31, 2021 •

edited

Loading

tudor Mar 31, 2021 •

edited

Loading

kristjanvalur Apr 2, 2021

kristjanvalur Apr 2, 2021

kristjanvalur Apr 15, 2021

tudor commented Aug 2, 2021

kristjanvalur commented Aug 3, 2021

tudor commented Aug 3, 2021

michael-grunder left a comment

michael-grunder Aug 9, 2021

michael-grunder Aug 9, 2021

michael-grunder Aug 9, 2021

michael-grunder Aug 9, 2021

michael-grunder Aug 9, 2021

michael-grunder Aug 9, 2021

zuiderkwast commented Mar 8, 2023

tudor commented Mar 9, 2023

kristjanvalur commented Mar 10, 2023

zuiderkwast commented Mar 10, 2023

kristjanvalur commented Mar 10, 2023

zuiderkwast commented Mar 10, 2023

zuiderkwast commented Jul 19, 2023

	redisCallback cb = {NULL, NULL, NULL, 0, NULL};
	redisCallback cb = {NULL, NULL, 0, NULL, NULL};

	printf("Get finalizer called\n");
	(void)privdata;
	printf("Get finalizer called\n");

	printf("Subscribe finalizer called\n");
	(void)privdata;
	printf("Subscribe finalizer called\n");

Add finalizer support to async Redis client #926

Are you sure you want to change the base?

Add finalizer support to async Redis client #926

Conversation

tudor commented Feb 19, 2021

tudor commented Mar 2, 2021

michael-grunder commented Mar 10, 2021

tudor commented Mar 19, 2021

tudor commented Mar 19, 2021

kristjanvalur commented Mar 30, 2021

tudor commented Mar 31, 2021

kristjanvalur Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

tudor Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tudor commented Aug 2, 2021

kristjanvalur commented Aug 3, 2021

tudor commented Aug 3, 2021

michael-grunder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuiderkwast commented Mar 8, 2023

tudor commented Mar 9, 2023

kristjanvalur commented Mar 10, 2023

zuiderkwast commented Mar 10, 2023

kristjanvalur commented Mar 10, 2023

zuiderkwast commented Mar 10, 2023

zuiderkwast commented Jul 19, 2023

kristjanvalur Mar 31, 2021 •

edited

Loading

tudor Mar 31, 2021 •

edited

Loading