From 6016973ac01b8b85a0361f885212b83443187304 Mon Sep 17 00:00:00 2001
From: Binbin <binloveplay1314@qq.com>
Date: Wed, 31 Jan 2024 19:10:19 +0800
Subject: [PATCH] Fix module assertion crash when timer and timeout are
 unlocked in the same event loop (#13015)

When we use a timer to unblock a client in module, if the timer
period and the block timeout are very close, they will unblock the
client in the same event loop, and it will trigger the assertion.
The reason is that in moduleBlockedClientTimedOut we will protect
against re-processing, so we don't actually call updateStatsOnUnblock
(see #12817), so we are not able to reset the c->duration.

The reason is unblockClientOnTimeout() didn't realize that bc had
been unblocked. We add a function to the module to determine if bc
is blocked, and then use it in unblockClientOnTimeout() to exit.

There is the stack:
```
beforeSleep
blockedBeforeSleep
handleBlockedClientsTimeout
checkBlockedClientTimeout
unblockClientOnTimeout
unblockClient
resetClient
-- assertion, crash the server
'c->duration == 0' is not true
```
---
 src/blocked.c                          | 3 +++
 src/module.c                           | 7 +++++++
 src/server.h                           | 1 +
 tests/unit/moduleapi/blockedclient.tcl | 6 ++++++
 4 files changed, 17 insertions(+)

diff --git a/src/blocked.c b/src/blocked.c
index 1bce1eaa7..2ada95760 100644
--- a/src/blocked.c
+++ b/src/blocked.c
@@ -707,6 +707,9 @@ static void moduleUnblockClientOnKey(client *c, robj *key) {
  * we want to remove the pending flag to indicate we already responded to the
  * command with timeout reply. */
 void unblockClientOnTimeout(client *c) {
+    /* The client has been unlocked (in the moduleUnblocked list), return ASAP. */
+    if (c->bstate.btype == BLOCKED_MODULE && isModuleClientUnblocked(c)) return;
+
     replyToBlockedClientTimedOut(c);
     if (c->flags & CLIENT_PENDING_COMMAND)
         c->flags &= ~CLIENT_PENDING_COMMAND;
diff --git a/src/module.c b/src/module.c
index b18377156..2fe2be8c1 100644
--- a/src/module.c
+++ b/src/module.c
@@ -7698,6 +7698,13 @@ void RM_LatencyAddSample(const char *event, mstime_t latency) {
  * https://redis.io/topics/modules-blocking-ops.
  * -------------------------------------------------------------------------- */
 
+/* Returns 1 if the client already in the moduleUnblocked list, 0 otherwise. */
+int isModuleClientUnblocked(client *c) {
+    RedisModuleBlockedClient *bc = c->bstate.module_blocked_handle;
+
+    return bc->unblocked == 1;
+}
+
 /* This is called from blocked.c in order to unblock a client: may be called
  * for multiple reasons while the client is in the middle of being blocked
  * because the client is terminated, but is also called for cleanup when a
diff --git a/src/server.h b/src/server.h
index 04829f197..b1e851f34 100644
--- a/src/server.h
+++ b/src/server.h
@@ -2532,6 +2532,7 @@ const char *moduleTypeModuleName(moduleType *mt);
 const char *moduleNameFromCommand(struct redisCommand *cmd);
 void moduleFreeContext(struct RedisModuleCtx *ctx);
 void moduleCallCommandUnblockedHandler(client *c);
+int isModuleClientUnblocked(client *c);
 void unblockClientFromModule(client *c);
 void moduleHandleBlockedClients(void);
 void moduleBlockedClientTimedOut(client *c, int from_module);
diff --git a/tests/unit/moduleapi/blockedclient.tcl b/tests/unit/moduleapi/blockedclient.tcl
index fdacb49b9..073d30895 100644
--- a/tests/unit/moduleapi/blockedclient.tcl
+++ b/tests/unit/moduleapi/blockedclient.tcl
@@ -290,6 +290,12 @@ foreach call_type {nested normal} {
         after 120
         $rd close
     }
+
+    test {block time is equal to timer period} {
+        # These time is equal, they will be unlocked in the same event loop,
+        # when the client is unlock, we will get the OK reply from timer.
+        assert_match "OK" [r unblock_by_timer 100 100]
+    }
     
     test "Unload the module - blockedclient" {
         assert_equal {OK} [r module unload blockedclient]