search

Nordix / hiredis-cluster

C client library for Valkey/Redis Cluster. This project is used and sponsored by Ericsson. It is a fork of the now unmaintained hiredis-vip.
BSD 3-Clause "New" or "Revised" License
86 stars 42 forks source link

redisClusterAsyncDisconnect behaviour when a redisAsyncContext object has seen some error #203

Closed rahulKrishnaM closed 6 months ago

rahulKrishnaM commented 6 months ago

Hi,

If there are some pending requests in hiredis queue, and if the application invokes redisClusterAsyncDisconnect() api, as per the below code block, redisAsyncDisconnect() would get called for all the nodes in the dictionary, which will then eventually invoke callbacks for all the pending requests in queue with NULL reply.

https://github.com/Nordix/hiredis-cluster/blob/88f9487fed252b8434e3e82f268c3ce5ce351831/hircluster.c#L4402C1-L4412C6

But if a particular node has seen some error (ac->err is true), invoking the redisClusterAsyncDisconnect() api is skipped.

if (ac == NULL || ac->err) { continue; }

Is there a reason why we do this? We actually in our application want all the callbacks to be invoked from hiredis. However, if a scenario as above happens, we will miss the callback from hiredis. Can you let us know if this check(ac->err) is present here for a reason?

cc: @bjosv

bjosv commented 6 months ago

Your reasoning sounds fair and I currently can't see the reason why either. The check was added in https://github.com/Nordix/hiredis-cluster/commit/b960bbf833523f0e0a284ecd8b15be5f457a9aaa I will look a bit more, but I also think we should changed this.