From 6b5b3ca4148b058210f7c32096a6d1201a2121d9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E9=83=AD=E4=BC=9F=E5=85=89?= <warriorguo@gmail.com>
Date: Tue, 1 Feb 2022 20:54:11 +0800
Subject: [PATCH] forbid module to unload when it holds ongoing timer (#10187)

This is done to avoid a crash when the timer fires after the module was unloaded.
Or memory leaks in case we wanted to just ignore the timer.
It'll cause the MODULE UNLOAD command to return with an error

Co-authored-by: sundb <sundbcn@gmail.com>
---
 runtest-moduleapi              |  1 +
 src/module.c                   | 26 ++++++++++++++++++++++
 tests/unit/moduleapi/timer.tcl | 40 +++++++++++++++++++++++++++++++++-
 3 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/runtest-moduleapi b/runtest-moduleapi
index 8b4b108de..d13425f81 100755
--- a/runtest-moduleapi
+++ b/runtest-moduleapi
@@ -45,5 +45,6 @@ $TCLSH tests/test_helper.tcl \
 --single unit/moduleapi/subcommands \
 --single unit/moduleapi/reply \
 --single unit/moduleapi/eventloop \
+--single unit/moduleapi/timer \
 "${@}"
 
diff --git a/src/module.c b/src/module.c
index 8c44422c6..204f49c8b 100644
--- a/src/module.c
+++ b/src/module.c
@@ -7427,6 +7427,24 @@ int RM_GetTimerInfo(RedisModuleCtx *ctx, RedisModuleTimerID id, uint64_t *remain
     return REDISMODULE_OK;
 }
 
+/* Query timers to see if any timer belongs to the module.
+ * Return 1 if any timer was found, otherwise 0 would be returned. */
+int moduleHoldsTimer(struct RedisModule *module) {
+    raxIterator iter;
+    int found = 0;
+    raxStart(&iter,Timers);
+    raxSeek(&iter,"^",NULL,0);
+    while (raxNext(&iter)) {
+        RedisModuleTimer *timer = iter.data;
+        if (timer->module == module) {
+            found = 1;
+            break;
+        }
+    }
+    raxStop(&iter);
+    return found;
+}
+
 /* --------------------------------------------------------------------------
  * ## Modules EventLoop API
  * --------------------------------------------------------------------------*/
@@ -10125,6 +10143,7 @@ int moduleLoad(const char *path, void **module_argv, int module_argc) {
  * * EBUSY: The module exports a new data type and can only be reloaded. 
  * * EPERM: The module exports APIs which are used by other module. 
  * * EAGAIN: The module has blocked clients. 
+ * * EINPROGRESS: The module holds timer not fired.
  * * ECANCELED: Unload module error.  */
 int moduleUnload(sds name) {
     struct RedisModule *module = dictFetchValue(modules,name);
@@ -10141,6 +10160,9 @@ int moduleUnload(sds name) {
     } else if (module->blocked_clients) {
         errno = EAGAIN;
         return C_ERR;
+    } else if (moduleHoldsTimer(module)) {
+        errno = EINPROGRESS;
+        return C_ERR;
     }
 
     /* Give module a chance to clean up. */
@@ -10345,6 +10367,10 @@ NULL
                 errmsg = "the module has blocked clients. "
                          "Please wait them unblocked and try again";
                 break;
+            case EINPROGRESS:
+                errmsg = "the module holds timer that is not fired. "
+                         "Please stop the timer or wait until it fires.";
+                break;
             default:
                 errmsg = "operation not possible.";
                 break;
diff --git a/tests/unit/moduleapi/timer.tcl b/tests/unit/moduleapi/timer.tcl
index c04f80b23..4e9dd0f09 100644
--- a/tests/unit/moduleapi/timer.tcl
+++ b/tests/unit/moduleapi/timer.tcl
@@ -26,6 +26,8 @@ start_server {tags {"modules"}} {
         assert_equal "timer-incr-key" [lindex $info 0]
         set remaining [lindex $info 1]
         assert {$remaining < 10000 && $remaining > 1}
+        # Stop the timer after get timer test
+        assert_equal 1 [r test.stoptimer $id]
     }
 
     test {RM_StopTimer: basic sanity} {
@@ -54,7 +56,43 @@ start_server {tags {"modules"}} {
         assert_equal {} [r test.gettimer $id]
     }
 
-    test "Unload the module - timer" {
+    test "Module can be unloaded when timer was finished" {
+        r set "timer-incr-key" 0
+        r test.createtimer 500 timer-incr-key
+
+        # Make sure the Timer has not been fired
+        assert_equal 0 [r get timer-incr-key]
+        # Module can not be unloaded since the timer was ongoing
+        catch {r module unload timer} err
+        assert_match {*the module holds timer that is not fired*} $err
+
+        # Wait to be sure timer has been finished
+        wait_for_condition 10 500 {
+            [r get timer-incr-key] == 1
+        } else {
+            fail "Timer not fired"
+        }
+
+        # Timer fired, can be unloaded now.
+        assert_equal {OK} [r module unload timer]
+    }
+
+    test "Module can be unloaded when timer was stopped" {
+        r module load $testmodule
+        r set "timer-incr-key" 0
+        set id [r test.createtimer 5000 timer-incr-key]
+
+        # Module can not be unloaded since the timer was ongoing
+        catch {r module unload timer} err
+        assert_match {*the module holds timer that is not fired*} $err
+
+        # Stop the timer
+        assert_equal 1 [r test.stoptimer $id]
+
+        # Make sure the Timer has not been fired
+        assert_equal 0 [r get timer-incr-key]
+
+        # Timer has stopped, can be unloaded now.
         assert_equal {OK} [r module unload timer]
     }
 }