From dba33a943d508bc5929db4950b4abadf6278ef02 Mon Sep 17 00:00:00 2001 From: guybe7 Date: Mon, 15 Mar 2021 20:19:57 +0100 Subject: [PATCH] Missing EXEC on modules propagation after failed EVAL execution (#8654) 1. moduleReplicateMultiIfNeeded should use server.in_eval like moduleHandlePropagationAfterCommandCallback 2. server.in_eval could have been set to 1 and not reset back to 0 (a lot of missed early-exits after in_eval is already 1) Note: The new assertions in processCommand cover (2) and I added two module tests to cover (1) Implications: If an EVAL that failed (and thus left server.in_eval=1) runs before a module command that replicates, the replication stream will contain MULTI (because moduleReplicateMultiIfNeeded used to check server.lua_caller which is NULL at this point) but not EXEC (because server.in_eval==1) This only affects modules as module.c the only user of server.in_eval. Affects versions 6.2.0, 6.2.1 --- src/module.c | 2 +- src/scripting.c | 5 ++- src/server.c | 10 ++++++ tests/unit/moduleapi/propagate.tcl | 49 ++++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 4 deletions(-) diff --git a/src/module.c b/src/module.c index c602dbddd..75d72cc50 100644 --- a/src/module.c +++ b/src/module.c @@ -1701,7 +1701,7 @@ int RM_ReplyWithLongDouble(RedisModuleCtx *ctx, long double ld) { void moduleReplicateMultiIfNeeded(RedisModuleCtx *ctx) { /* Skip this if client explicitly wrap the command with MULTI, or if * the module command was called by a script. */ - if (server.lua_caller || server.in_exec) return; + if (server.in_eval || server.in_exec) return; /* If we already emitted MULTI return ASAP. */ if (server.propagate_in_transaction) return; /* If this is a thread safe context, we do not want to wrap commands diff --git a/src/scripting.c b/src/scripting.c index 6830e7a70..345feda2f 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -1498,7 +1498,6 @@ void evalGenericCommand(client *c, int evalsha) { server.lua_replicate_commands = server.lua_always_replicate_commands; server.lua_multi_emitted = 0; server.lua_repl = PROPAGATE_AOF|PROPAGATE_REPL; - server.in_eval = 1; /* Get the number of arguments that are keys */ if (getLongLongFromObjectOrReply(c,c->argv[2],&numkeys,NULL) != C_OK) @@ -1570,6 +1569,7 @@ void evalGenericCommand(client *c, int evalsha) { * * If we are debugging, we set instead a "line" hook so that the * debugger is call-back at every line executed by the script. */ + server.in_eval = 1; server.lua_caller = c; server.lua_cur_script = funcname + 2; server.lua_time_start = mstime(); @@ -1602,6 +1602,7 @@ void evalGenericCommand(client *c, int evalsha) { if (server.masterhost && server.master) queueClientForReprocessing(server.master); } + server.in_eval = 0; server.lua_caller = NULL; server.lua_cur_script = NULL; @@ -1678,8 +1679,6 @@ void evalGenericCommand(client *c, int evalsha) { forceCommandPropagation(c,PROPAGATE_REPL|PROPAGATE_AOF); } } - - server.in_eval = 0; } void evalCommand(client *c) { diff --git a/src/server.c b/src/server.c index 7e0111cea..5cfb13bbb 100644 --- a/src/server.c +++ b/src/server.c @@ -3916,6 +3916,16 @@ static int cmdHasMovableKeys(struct redisCommand *cmd) { * other operations can be performed by the caller. Otherwise * if C_ERR is returned the client was destroyed (i.e. after QUIT). */ int processCommand(client *c) { + if (!server.lua_timedout) { + /* Both EXEC and EVAL call call() directly so there should be + * no way in_exec or in_eval or propagate_in_transaction is 1. + * That is unless lua_timedout, in which case client may run + * some commands. */ + serverAssert(!server.propagate_in_transaction); + serverAssert(!server.in_exec); + serverAssert(!server.in_eval); + } + moduleCallCommandFilters(c); /* The QUIT command is handled separately. Normal command procs will diff --git a/tests/unit/moduleapi/propagate.tcl b/tests/unit/moduleapi/propagate.tcl index 1277d3970..108637dd9 100644 --- a/tests/unit/moduleapi/propagate.tcl +++ b/tests/unit/moduleapi/propagate.tcl @@ -141,6 +141,55 @@ tags "modules" { close_replication_stream $repl } + test {module propagates from from command after good EVAL} { + set repl [attach_to_replication_stream] + + assert_equal [ $master eval { return "hello" } 0 ] {hello} + $master propagate-test.simple + $master propagate-test.mixed + + # Note the 'after-call' propagation below is out of order (known limitation) + assert_replication_stream $repl { + {select *} + {multi} + {incr counter-1} + {incr counter-2} + {exec} + {multi} + {incr using-call} + {incr after-call} + {incr counter-1} + {incr counter-2} + {exec} + } + close_replication_stream $repl + } + + test {module propagates from from command after bad EVAL} { + set repl [attach_to_replication_stream] + + catch { $master eval { return "hello" } -12 } e + assert_equal $e {ERR Number of keys can't be negative} + $master propagate-test.simple + $master propagate-test.mixed + + # Note the 'after-call' propagation below is out of order (known limitation) + assert_replication_stream $repl { + {select *} + {multi} + {incr counter-1} + {incr counter-2} + {exec} + {multi} + {incr using-call} + {incr after-call} + {incr counter-1} + {incr counter-2} + {exec} + } + close_replication_stream $repl + } + test {module propagates from from multi-exec} { set repl [attach_to_replication_stream]