Unverified Commit dba33a94 authored by guybe7's avatar guybe7 Committed by GitHub
Browse files

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
parent 84d056d0
...@@ -1701,7 +1701,7 @@ int RM_ReplyWithLongDouble(RedisModuleCtx *ctx, long double ld) { ...@@ -1701,7 +1701,7 @@ int RM_ReplyWithLongDouble(RedisModuleCtx *ctx, long double ld) {
void moduleReplicateMultiIfNeeded(RedisModuleCtx *ctx) { void moduleReplicateMultiIfNeeded(RedisModuleCtx *ctx) {
/* Skip this if client explicitly wrap the command with MULTI, or if /* Skip this if client explicitly wrap the command with MULTI, or if
* the module command was called by a script. */ * 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 we already emitted MULTI return ASAP. */
if (server.propagate_in_transaction) return; if (server.propagate_in_transaction) return;
/* If this is a thread safe context, we do not want to wrap commands /* If this is a thread safe context, we do not want to wrap commands
......
...@@ -1498,7 +1498,6 @@ void evalGenericCommand(client *c, int evalsha) { ...@@ -1498,7 +1498,6 @@ void evalGenericCommand(client *c, int evalsha) {
server.lua_replicate_commands = server.lua_always_replicate_commands; server.lua_replicate_commands = server.lua_always_replicate_commands;
server.lua_multi_emitted = 0; server.lua_multi_emitted = 0;
server.lua_repl = PROPAGATE_AOF|PROPAGATE_REPL; server.lua_repl = PROPAGATE_AOF|PROPAGATE_REPL;
server.in_eval = 1;
/* Get the number of arguments that are keys */ /* Get the number of arguments that are keys */
if (getLongLongFromObjectOrReply(c,c->argv[2],&numkeys,NULL) != C_OK) if (getLongLongFromObjectOrReply(c,c->argv[2],&numkeys,NULL) != C_OK)
...@@ -1570,6 +1569,7 @@ void evalGenericCommand(client *c, int evalsha) { ...@@ -1570,6 +1569,7 @@ void evalGenericCommand(client *c, int evalsha) {
* *
* If we are debugging, we set instead a "line" hook so that the * If we are debugging, we set instead a "line" hook so that the
* debugger is call-back at every line executed by the script. */ * debugger is call-back at every line executed by the script. */
server.in_eval = 1;
server.lua_caller = c; server.lua_caller = c;
server.lua_cur_script = funcname + 2; server.lua_cur_script = funcname + 2;
server.lua_time_start = mstime(); server.lua_time_start = mstime();
...@@ -1602,6 +1602,7 @@ void evalGenericCommand(client *c, int evalsha) { ...@@ -1602,6 +1602,7 @@ void evalGenericCommand(client *c, int evalsha) {
if (server.masterhost && server.master) if (server.masterhost && server.master)
queueClientForReprocessing(server.master); queueClientForReprocessing(server.master);
} }
server.in_eval = 0;
server.lua_caller = NULL; server.lua_caller = NULL;
server.lua_cur_script = NULL; server.lua_cur_script = NULL;
...@@ -1678,8 +1679,6 @@ void evalGenericCommand(client *c, int evalsha) { ...@@ -1678,8 +1679,6 @@ void evalGenericCommand(client *c, int evalsha) {
forceCommandPropagation(c,PROPAGATE_REPL|PROPAGATE_AOF); forceCommandPropagation(c,PROPAGATE_REPL|PROPAGATE_AOF);
} }
} }
server.in_eval = 0;
} }
void evalCommand(client *c) { void evalCommand(client *c) {
......
...@@ -3916,6 +3916,16 @@ static int cmdHasMovableKeys(struct redisCommand *cmd) { ...@@ -3916,6 +3916,16 @@ static int cmdHasMovableKeys(struct redisCommand *cmd) {
* other operations can be performed by the caller. Otherwise * other operations can be performed by the caller. Otherwise
* if C_ERR is returned the client was destroyed (i.e. after QUIT). */ * if C_ERR is returned the client was destroyed (i.e. after QUIT). */
int processCommand(client *c) { 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); moduleCallCommandFilters(c);
/* The QUIT command is handled separately. Normal command procs will /* The QUIT command is handled separately. Normal command procs will
......
...@@ -141,6 +141,55 @@ tags "modules" { ...@@ -141,6 +141,55 @@ tags "modules" {
close_replication_stream $repl 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} { test {module propagates from from multi-exec} {
set repl [attach_to_replication_stream] set repl [attach_to_replication_stream]
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment