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

Prevent slotmap updates during redisClusterAsyncFree() #195

Closed bjosv closed 10 months ago

bjosv commented 10 months ago

When a node and its asynchronous hiredis context are freed all pending callbacks are executed. By clearing cc->nodes before releasing the dict we prevent a slotmap update command callback to trigger additional slotmap updates when calling updateSlotMapAsync().

Attempted to add a testcase for this scenario, but wouldn't fit in the test setup.

Fixes #194

zuiderkwast commented 10 months ago

So IIUC, when the redisAsyncContext is freed, the clusterNodesReplyCallback is called with reply == NULL, which triggers another call to updateSlotMapAsync, which fails and sets the error string to "no nodes added"? It works, I think.

Isn't REDIS_FREEING already set in ac->c->flags here (set by hiredis) so we could check this and return earlier instead of calling updateSlotMapAsync again? Or can that happen for an individual redis context when we're not freeing the cluster context? Perhaps we should not rely on it?

bjosv commented 10 months ago

Isn't REDIS_FREEING already set in ac->c->flags here (set by hiredis) so we could check this and return earlier instead of calling updateSlotMapAsync again? Or can that happen for an individual redis context when we're not freeing the cluster context? Perhaps we should not rely on it?

When a individual redisClusterNode is removed during a slotmap-update its redis context is also flagged with REDIS_FREEING. Since we don't allow concurrent slotmap updates we shouldn't be able to remove a single node with an outstanding slotmap update command, so possibly that wouldn't be a problem. I didn't want to add additional dependencies to hiredis internal flags, but by preventing any type of callback to work on a inconsistent/corrupt nodes dict the already existing check fixed the problem.