From 9dc6f93e9d0aadc7a8d6cd3617fa99bdfbba10a5 Mon Sep 17 00:00:00 2001
From: Binbin <binloveplay1314@qq.com>
Date: Fri, 16 Jun 2023 20:39:00 +0800
Subject: [PATCH] Add command being unblocked cause another command to get
 unblocked execution order test (#12324)

* Add command being unblocked cause another command to get unblocked execution order test

In #12301, we observed that if the
`while(listLength(server.ready_keys) != 0)`
in handleClientsBlockedOnKeys is changed to
`if(listLength(server.ready_keys) != 0)`,
the order of command execution will change.

It is wrong to change that. It means that if a command
being unblocked causes another command to get unblocked
(like a BLMOVE would do), then the new unblocked command
will wait for later to get processed rather than right away.

It'll not have any real implication if we change that since
we do call handleClientsBlockedOnKeys in beforeSleep again,
and redis will still behave correctly, but we don't change that.

An example:
1. $rd1 blmove src{t} dst{t} left right 0
2. $rd2 blmove dst{t} src{t} right left 0
3. $rd3 set key1{t}, $rd3 lpush src{t}, $rd3 set key2{t} in a pipeline

The correct order would be:
1. set key1{t}
2. lpush src{t}
3. lmove src{t} dst{t} left right
4. lmove dst{t} src{t} right left
5. set key2{t}

The wrong order would be:
1. set key1{t}
2. lpush src{t}
3. lmove src{t} dst{t} left right
4. set key2{t}
5. lmove dst{t} src{t} right left

This PR adds corresponding test to cover it.

* Add comment near while(listLength(server.ready_keys) != 0)
---
 src/blocked.c            |  3 +++
 tests/unit/type/list.tcl | 48 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/src/blocked.c b/src/blocked.c
index a7548ce98..fc2b5cdc6 100644
--- a/src/blocked.c
+++ b/src/blocked.c
@@ -325,6 +325,9 @@ void handleClientsBlockedOnKeys(void) {
      * (i.e. not from call(), module context, etc.) */
     serverAssert(server.also_propagate.numops == 0);
 
+    /* If a command being unblocked causes another command to get unblocked,
+     * like a BLMOVE would do, then the new unblocked command will get processed
+     * right away rather than wait for later. */
     while(listLength(server.ready_keys) != 0) {
         list *l;
 
diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl
index a57e5df3e..993b6d135 100644
--- a/tests/unit/type/list.tcl
+++ b/tests/unit/type/list.tcl
@@ -2312,4 +2312,52 @@ foreach {pop} {BLPOP BLMPOP_RIGHT} {
         
         $rd close
     }
+
+    test {Command being unblocked cause another command to get unblocked execution order test} {
+        r del src{t} dst{t} key1{t} key2{t} key3{t}
+        set repl [attach_to_replication_stream]
+
+        set rd1 [redis_deferring_client]
+        set rd2 [redis_deferring_client]
+        set rd3 [redis_deferring_client]
+
+        $rd1 blmove src{t} dst{t} left right 0
+        wait_for_blocked_clients_count 1
+
+        $rd2 blmove dst{t} src{t} right left 0
+        wait_for_blocked_clients_count 2
+
+        # Create a pipeline of commands that will be processed in one socket read.
+        # Insert two set commands before and after lpush to observe the execution order.
+        set buf ""
+        append buf "set key1{t} value1\r\n"
+        append buf "lpush src{t} dummy\r\n"
+        append buf "set key2{t} value2\r\n"
+        $rd3 write $buf
+        $rd3 flush
+
+        wait_for_blocked_clients_count 0
+
+        r set key3{t} value3
+
+        # If a command being unblocked causes another command to get unblocked, like a BLMOVE would do,
+        # then the new unblocked command will get processed right away rather than wait for later.
+        # If the set command occurs between two lmove commands, the results are not as expected.
+        assert_replication_stream $repl {
+            {select *}
+            {set key1{t} value1}
+            {lpush src{t} dummy}
+            {lmove src{t} dst{t} left right}
+            {lmove dst{t} src{t} right left}
+            {set key2{t} value2}
+            {set key3{t} value3}
+        }
+
+        $rd1 close
+        $rd2 close
+        $rd3 close
+
+        close_replication_stream $repl
+    } {} {needs:repl}
+
 } ;# stop servers