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

@@ -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 \
 "${@}"
 

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;

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]
     }
 }