Change in redisReply memory ownership for redisAsyncCommand

[ewencp]

Commit 3ce8d5b changed the behavior of async callbacks by always freeing memory after the callback instead of leaving it to the callback. This change may not have been noticed since it looks like the samples using the async API weren't even freeing memory properly before the change.

Either this was intended, in which case the readme should be updated to reflect the change in behavior, e.g.:

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d41db8a..73e46b2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,3 +14,6 @@

 * See commit log.

+* The asynchronous API now free's replies automatically: the callback
+  does not own the redisReply and should not call freeReplyObject on
+  it.
diff --git a/README.md b/README.md
index 6cf7b1c..a58101c 100644
--- a/README.md
+++ b/README.md
@@ -286,7 +286,8 @@ is being disconnected per user-request, no new commands may be added to the outp
 returned on calls to the `redisAsyncCommand` family.

 If the reply for a command with a `NULL` callback is read, it is immediately free'd. When the callback
-for a command is non-`NULL`, it is responsible for cleaning up the reply.
+for a command is non-`NULL`, the memory is free'd immediately following the callback: the reply is only
+valid for the duration of the callback.

 All pending callbacks are called with a `NULL` reply when the context encountered an error.

If this was not intended, the fix is probably more substantial since the commit that made the relevant change suggests the pub/sub code uses this code path and probably relies on it to free the replies. However, I don't haven't used the pub/sub code at all or looked at its implementation, so I am not certain about that.

[janoberst]

+1 on this. Just caused me a major headache, since the readme says that I have to take care of freeing replies.

I'm not sure what the best option would be here, since sometimes I don't want to take care of freeing objects myself. But other times I want to keep a copy for later use.

[pietern]

Thanks for submitting this issue. I also would expect this behavior to be different right now (how viewpoints change over time...). I'm slowly rewriting parts of hiredis (see the ng branch), so let's use this issue to discuss what the right behavior should be.

For the usage described by @janoberst I'm a proponent of only free'ing the reply if there is no callback to handle it.

[janoberst]

Cool to hear that you're rewriting parts of hiredis.. I'm just using a version that has the freeObject line commented out and I'm taking care of freeing the reply struct myself. That has been working pretty good so far.

My use-case is this:

• I have a function in my application that needs the result of multiple Redis requests from multiple servers in my cluster.
• Only once all of those requests are finished I want my callback to run, because it needs to work on the data from all those calls.
• Instead of my application's callback function I pass a proxy callback to the redisAsyncCommand function. After each Redis command finishes, the proxy function adds the pointers to all redis replies to a list for later use.
• Once the proxy sees that all redis calls have returned it calls my application.
• In the application callback I examine all the replies and free them eventually.

Therefore I need to hold on to the redisReply struct even after my proxy callback function has finished. On the other hand, I have multiple "simple" calls where I don't need the redisReply function once the callback has finished. I'd say 90% of the time I don't need to hold on to the reply.

I will implement this on my branch in a bit: Add another kind of callback function that sets a flag in the redisCallback struct. Upon calling the callback, hiredis checks if the FREE_REPLY_OBJECT flag is set and acts accordingly. This wouldn't change any existing API and only require a bit of memory in the redisCallback struct.

Let me know if it would help at all if I attached my changes here or opened a pull request...

[ScottKevill]

Callbacks definitely need to be able to retain the reply. Given that the reply is deep object and non-trivial to duplicate, I think it was a mistake to ever prevent a callback retaining it.

That said, I don't see any advantage of adding a flag either.

I have no problem with all callbacks required to free the reply themselves. Only if no callback is supplied should it automatically free. It's the principle of least surprise. This is serious enough that it shouldn't wait for ng, imho.

[michael-grunder]

I'm going to close this very old issue but after v1.0.0 we can look at making this more flexible.

One option is to use refcounts for the redisReply object which has the advantage of allowing code to choose whether or not they want to retain the reply (simply by incrementing the refcount), and it's cheap.