Add NO_MULTI flag to MULTI / WATCH to make them abort transactions

Currently, for nested multi or executing watch in multi, we will return an error but we will not abort the transaction. ``` 127.0.0.1:6379> multi OK 127.0.0.1:6379(TX)> multi (error) ERR MULTI calls can not be nested127.0.0.1:6379(TX)> set key value QUEUED 127.0.0.1:6379(TX)> exec 1) OK 127.0.0.1:6379> multi OK 127.0.0.1:6379(TX)> watch key (error) ERR WATCH inside MULTI is not allowed 127.0.0.1:6379(TX)> set key value QUEUED 127.0.0.1:6379(TX)> exec 1) OK ``` In theory, this is also a syntax error, an unexpected behavior should abort the transaction. Add the NO_MULTI flag to them so that they will be rejected in processCommand and rejectCommand will abort the transaction. Signed-off-by: Binbin <[email protected]>
enjoy-binbin · Jul 1, 2024 · 5a75d7e · 5a75d7e
1 parent 2420881
commit 5a75d7e
Show file tree

Hide file tree

Showing 5 changed files with 10 additions and 21 deletions.
diff --git a/src/commands.def b/src/commands.def
@@ -11022,8 +11022,8 @@ struct COMMAND_STRUCT serverCommandTable[] = {
 /* transactions */
 {MAKE_CMD("discard","Discards a transaction.","O(N), when N is the number of queued commands","2.0.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,DISCARD_History,0,DISCARD_Tips,0,discardCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,DISCARD_Keyspecs,0,NULL,0)},
 {MAKE_CMD("exec","Executes all commands in a transaction.","Depends on commands in the transaction","1.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,EXEC_History,0,EXEC_Tips,0,execCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SKIP_SLOWLOG,ACL_CATEGORY_TRANSACTION,EXEC_Keyspecs,0,NULL,0)},
-{MAKE_CMD("multi","Starts a transaction.","O(1)","1.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,MULTI_History,0,MULTI_Tips,0,multiCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,MULTI_Keyspecs,0,NULL,0)},
+{MAKE_CMD("multi","Starts a transaction.","O(1)","1.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,MULTI_History,0,MULTI_Tips,0,multiCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_NO_MULTI|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,MULTI_Keyspecs,0,NULL,0)},
 {MAKE_CMD("unwatch","Forgets about watched keys of a transaction.","O(1)","2.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,UNWATCH_History,0,UNWATCH_Tips,0,unwatchCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,UNWATCH_Keyspecs,0,NULL,0)},
-{MAKE_CMD("watch","Monitors changes to keys to determine the execution of a transaction.","O(1) for every key.","2.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,WATCH_History,0,WATCH_Tips,0,watchCommand,-2,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,WATCH_Keyspecs,1,NULL,1),.args=WATCH_Args},
+{MAKE_CMD("watch","Monitors changes to keys to determine the execution of a transaction.","O(1) for every key.","2.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,WATCH_History,0,WATCH_Tips,0,watchCommand,-2,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_NO_MULTI|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,WATCH_Keyspecs,1,NULL,1),.args=WATCH_Args},
 {0}
 };
diff --git a/src/commands/multi.json b/src/commands/multi.json
@@ -11,6 +11,7 @@
             "LOADING",
             "STALE",
             "FAST",
+            "NO_MULTI",
             "ALLOW_BUSY"
         ],
         "acl_categories": [

diff --git a/src/commands/watch.json b/src/commands/watch.json
@@ -11,6 +11,7 @@
             "LOADING",
             "STALE",
             "FAST",
+            "NO_MULTI",
             "ALLOW_BUSY"
         ],
         "acl_categories": [

diff --git a/src/multi.c b/src/multi.c
@@ -109,12 +109,7 @@ void flagTransaction(client *c) {
 }
 
 void multiCommand(client *c) {
-    if (c->flag.multi) {
-        addReplyError(c, "MULTI calls can not be nested");
-        return;
-    }
     c->flag.multi = 1;
-
     addReply(c, shared.ok);
 }
 
@@ -459,10 +454,6 @@ void touchAllWatchedKeysInDb(serverDb *emptied, serverDb *replaced_with) {
 void watchCommand(client *c) {
     int j;
 
-    if (c->flag.multi) {
-        addReplyError(c, "WATCH inside MULTI is not allowed");
-        return;
-    }
     /* No point in watching if the client is already dirty. */
     if (c->flag.dirty_cas) {
         addReply(c, shared.ok);

diff --git a/tests/unit/multi.tcl b/tests/unit/multi.tcl
@@ -34,12 +34,10 @@ start_server {tags {"multi"}} {
     } {QUEUED OK {a b c}}
 
     test {Nested MULTI are not allowed} {
-        set err {}
         r multi
-        catch {[r multi]} err
-        r exec
-        set _ $err
-    } {*ERR MULTI*}
+        assert_error "ERR*" {r multi}
+        assert_error "EXECABORT*" {r exec}
+    }
 
     test {MULTI where commands alter argc/argv} {
         r sadd myset a
@@ -49,12 +47,10 @@ start_server {tags {"multi"}} {
     } {a 0}
 
     test {WATCH inside MULTI is not allowed} {
-        set err {}
         r multi
-        catch {[r watch x]} err
-        r exec
-        set _ $err
-    } {*ERR WATCH*}
+        assert_error "ERR*" {r watch}
+        assert_error "EXECABORT*" {r exec}
+    }
 
     test {EXEC fails if there are errors while queueing commands #1} {
         r del foo1{t} foo2{t}