Merge pull request #6699 from guybe7/module_blocked_on_key_timeout_memleak

Modules: Fix blocked-client-related memory leak

2 files changed, 42 insertions, 6 deletions

diff --git a/tests/modules/blockonkeys.c b/tests/modules/blockonkeys.c
index 959918b1c..10dc65b1a 100644
--- a/tests/modules/blockonkeys.c
+++ b/tests/modules/blockonkeys.c
@@ -172,13 +172,13 @@ int bpopgt_reply_callback(RedisModuleCtx *ctx, RedisModuleString **argv, int arg
     REDISMODULE_NOT_USED(argv);
     REDISMODULE_NOT_USED(argc);
     RedisModuleString *keyname = RedisModule_GetBlockedClientReadyKey(ctx);
-    long long gt = (long long)RedisModule_GetBlockedClientPrivateData(ctx);
+    long long *pgt = RedisModule_GetBlockedClientPrivateData(ctx);
 
     fsl_t *fsl;
     if (!get_fsl(ctx, keyname, REDISMODULE_READ, 0, &fsl, 0))
         return REDISMODULE_ERR;
 
-    if (!fsl || fsl->list[fsl->length-1] <= gt)
+    if (!fsl || fsl->list[fsl->length-1] <= *pgt)
         return REDISMODULE_ERR;
 
     RedisModule_ReplyWithLongLong(ctx, fsl->list[--fsl->length]);
@@ -192,10 +192,8 @@ int bpopgt_timeout_callback(RedisModuleCtx *ctx, RedisModuleString **argv, int a
 }
 
 void bpopgt_free_privdata(RedisModuleCtx *ctx, void *privdata) {
-    /* Nothing to do because privdata is actually a 'long long',
-     * not a pointer to the heap */
     REDISMODULE_NOT_USED(ctx);
-    REDISMODULE_NOT_USED(privdata);
+    RedisModule_Free(privdata);
 }
 
 /* FSL.BPOPGT <key> <gt> <timeout> - Block clients until list has an element greater than <gt>.
@@ -217,9 +215,12 @@ int fsl_bpopgt(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
         return REDISMODULE_OK;
 
     if (!fsl || fsl->list[fsl->length-1] <= gt) {
+        /* We use malloc so the tests in blockedonkeys.tcl can check for memory leaks */
+        long long *pgt = RedisModule_Alloc(sizeof(long long));
+        *pgt = gt;
         /* Key is empty or has <2 elements, we must block */
         RedisModule_BlockClientOnKeys(ctx, bpopgt_reply_callback, bpopgt_timeout_callback,
-                                      bpopgt_free_privdata, timeout, &argv[1], 1, (void*)gt);
+                                      bpopgt_free_privdata, timeout, &argv[1], 1, pgt);
     } else {
         RedisModule_ReplyWithLongLong(ctx, fsl->list[--fsl->length]);
     }
diff --git a/tests/unit/moduleapi/blockonkeys.tcl b/tests/unit/moduleapi/blockonkeys.tcl
index cb99ab1c9..b380227e0 100644
--- a/tests/unit/moduleapi/blockonkeys.tcl
+++ b/tests/unit/moduleapi/blockonkeys.tcl
@@ -45,18 +45,24 @@ start_server {tags {"modules"}} {
     test {Module client blocked on keys (with metadata): Timeout} {
         r del k
         set rd [redis_deferring_client]
+        $rd client id
+        set cid [$rd read]
         r fsl.push k 33
         $rd fsl.bpopgt k 35 1
         assert_equal {Request timedout} [$rd read]
+        r client kill id $cid ;# try to smoke-out client-related memory leak
     }
 
     test {Module client blocked on keys (with metadata): Blocked, case 1} {
         r del k
         set rd [redis_deferring_client]
+        $rd client id
+        set cid [$rd read]
         r fsl.push k 33
         $rd fsl.bpopgt k 33 0
         r fsl.push k 34
         assert_equal {34} [$rd read]
+        r client kill id $cid ;# try to smoke-out client-related memory leak
     }
 
     test {Module client blocked on keys (with metadata): Blocked, case 2} {
@@ -70,6 +76,35 @@ start_server {tags {"modules"}} {
         assert_equal {36} [$rd read]
     }
 
+    test {Module client blocked on keys (with metadata): Blocked, CLIENT KILL} {
+        r del k
+        set rd [redis_deferring_client]
+        $rd client id
+        set cid [$rd read]
+        $rd fsl.bpopgt k 35 0
+        r client kill id $cid ;# try to smoke-out client-related memory leak
+    }
+
+    test {Module client blocked on keys (with metadata): Blocked, CLIENT UNBLOCK TIMEOUT} {
+        r del k
+        set rd [redis_deferring_client]
+        $rd client id
+        set cid [$rd read]
+        $rd fsl.bpopgt k 35 0
+        r client unblock $cid timeout ;# try to smoke-out client-related memory leak
+        assert_equal {Request timedout} [$rd read]
+    }
+
+    test {Module client blocked on keys (with metadata): Blocked, CLIENT UNBLOCK ERROR} {
+        r del k
+        set rd [redis_deferring_client]
+        $rd client id
+        set cid [$rd read]
+        $rd fsl.bpopgt k 35 0
+        r client unblock $cid error ;# try to smoke-out client-related memory leak
+        assert_error "*unblocked*" {$rd read}
+    }
+
     test {Module client blocked on keys does not wake up on wrong type} {
         r del k
         set rd [redis_deferring_client]