Merge pull request #15 from yossigo/fix-raftize-reentrancy

Fix raftize re-entrancy issues (#13, #14).

Author: yossigo

common.c

@@ -2,6 +2,8 @@
 
 #include "redisraft.h"
 
+int redis_raft_in_rm_call = 0;
+
 const char *getStateStr(RedisRaftCtx *rr)
 {
     static const char *state_str[] = { "uninitialized", "up", "loading", "joining" };
@@ -144,6 +146,13 @@ static void raftize_commands(RedisModuleCommandFilterCtx *filter)
         NULL
     };
 
+    /* If we're intercepting an RM_Call() processing a Raft entry,
+     * skip.
+     */
+    if (checkInRedisModuleCall()) {
+        return;
+    }
+
     size_t cmdname_len;
     const char *cmdname = RedisModule_StringPtrLen(
             RedisModule_CommandFilterArgGet(filter, 0), &cmdname_len);

raft.c

@@ -154,8 +154,10 @@ static void executeRaftRedisCommandArray(RaftRedisCommandArray *array,
             continue;
         }
 
+        enterRedisModuleCall();
         RedisModuleCallReply *reply = RedisModule_Call(
                 ctx, cmd, "v", &c->argv[1], c->argc - 1);
+        exitRedisModuleCall();
 
         if (reply_ctx) {
             if (reply) {

redisraft.h

@@ -361,6 +361,33 @@ typedef struct SnapshotResult {
     char err[256];
 } SnapshotResult;
 
+/* Command filtering re-entrancy counter handling.
+ *
+ * This mechanim tracks calls from Redis Raft into Redis and used by the
+ * command filtering hook to avoid raftizing commands as they're pushed from the log
+ * to the FSM.
+ *
+ * Redis Module API provides the REDISMODULE_CMDFILTER_NOSELF flag which does
+ * the same thing, but does not apply to executions from a thread safe context.
+ *
+ * This must wrap every call to RedisModule_Call(), after the Redis lock has been
+ * acquired, and unless the called command is known to be excluded from raftizing.
+ */
+
+extern int redis_raft_in_rm_call;   /* defined in common.c */
+
+static void inline enterRedisModuleCall(void) {
+    redis_raft_in_rm_call++;
+}
+
+static void inline exitRedisModuleCall(void) {
+    redis_raft_in_rm_call--;
+}
+
+static int inline checkInRedisModuleCall(void) {
+    return redis_raft_in_rm_call;
+}
+
 /* common.c */
 const char *getStateStr(RedisRaftCtx *rr);
 const char *raft_logtype_str(int type);

tests/integration/test_sanity.py

@@ -2,7 +2,7 @@
 from pytest import raises, skip
 from fixtures import cluster
 from sandbox import RedisRaft
-
+import time
 
 def test_add_node_as_a_single_leader(cluster):
     """
@@ -182,3 +182,34 @@ def test_raftize_does_not_affect_lua(cluster):
     assert r1.client.get('key1') == b'value1'
     assert r1.client.get('key2') == b'value2'
     assert r1.client.get('key3') == b'value3'
+
+def test_proxying_with_raftize(cluster):
+    """
+    Test follower proxy mode together with raftize enabled.
+
+    This is a regression test for issues #13, #14 and basically ensures
+    that command filtering does not trigger re-entrancy when processing
+    a log entry over a thread safe context.
+    """
+    cluster.create(3)
+    assert cluster.leader == 1
+
+    assert cluster.node(1).client.execute_command(
+        'RAFT.CONFIG', 'SET', 'follower-proxy', 'yes') == b'OK'
+    assert cluster.node(2).client.execute_command(
+        'RAFT.CONFIG', 'SET', 'follower-proxy', 'yes') == b'OK'
+    assert cluster.node(3).client.execute_command(
+        'RAFT.CONFIG', 'SET', 'follower-proxy', 'yes') == b'OK'
+    assert cluster.node(1).client.execute_command(
+        'RAFT.CONFIG', 'SET', 'raftize-all-commands', 'yes') == b'OK'
+    assert cluster.node(2).client.execute_command(
+        'RAFT.CONFIG', 'SET', 'raftize-all-commands', 'yes') == b'OK'
+    assert cluster.node(3).client.execute_command(
+        'RAFT.CONFIG', 'SET', 'raftize-all-commands', 'yes') == b'OK'
+
+    assert cluster.node(1).raft_exec('RPUSH', 'list-a', 'x') == 1
+    assert cluster.node(1).raft_exec('RPUSH', 'list-a', 'x') == 2
+    assert cluster.node(1).raft_exec('RPUSH', 'list-a', 'x') == 3
+
+    time.sleep(1)
+    assert cluster.node(1).raft_info()['current_index'] == 8