From be352633a07552d0c3893ffa096b3732347ea378 Mon Sep 17 00:00:00 2001
From: Oran Agra <oran@redislabs.com>
Date: Thu, 24 Oct 2019 09:45:25 +0300
Subject: Make module tests pass with valgrind, and fix a leak in diskless load

---
 src/rdb.c                        |  6 +++++-
 tests/modules/commandfilter.c    |  5 +++++
 tests/modules/testrdb.c          |  8 +++++++
 tests/unit/moduleapi/testrdb.tcl | 45 ++++++++++++++++------------------------
 4 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/src/rdb.c b/src/rdb.c
index 2406ea88a..cbaa428a1 100644
--- a/src/rdb.c
+++ b/src/rdb.c
@@ -2172,7 +2172,11 @@ int rdbLoadRio(rio *rdb, rdbSaveInfo *rsi, int loading_aof) {
         /* Read key */
         if ((key = rdbLoadStringObject(rdb)) == NULL) goto eoferr;
         /* Read value */
-        if ((val = rdbLoadObject(type,rdb,key)) == NULL) goto eoferr;
+        if ((val = rdbLoadObject(type,rdb,key)) == NULL) {
+            decrRefCount(key);
+            goto eoferr;
+        }
+
         /* Check if the key already expired. This function is used when loading
          * an RDB file from disk, either at startup, or when an RDB was
          * received from the master. In the latter case, the master is
diff --git a/tests/modules/commandfilter.c b/tests/modules/commandfilter.c
index d25d49c44..571ed1701 100644
--- a/tests/modules/commandfilter.c
+++ b/tests/modules/commandfilter.c
@@ -147,3 +147,8 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
 
     return REDISMODULE_OK;
 }
+
+int RedisModule_OnUnload(RedisModuleCtx *ctx) {
+    RedisModule_FreeString(ctx, log_key_name);
+    return REDISMODULE_OK;
+}
diff --git a/tests/modules/testrdb.c b/tests/modules/testrdb.c
index d73c8bfd3..eb8d1a999 100644
--- a/tests/modules/testrdb.c
+++ b/tests/modules/testrdb.c
@@ -238,3 +238,11 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
 
     return REDISMODULE_OK;
 }
+
+int RedisModule_OnUnload(RedisModuleCtx *ctx) {
+    if (before_str)
+        RedisModule_FreeString(ctx, before_str);
+    if (after_str)
+        RedisModule_FreeString(ctx, after_str);
+    return REDISMODULE_OK;
+}
diff --git a/tests/unit/moduleapi/testrdb.tcl b/tests/unit/moduleapi/testrdb.tcl
index c72570002..a93b34b69 100644
--- a/tests/unit/moduleapi/testrdb.tcl
+++ b/tests/unit/moduleapi/testrdb.tcl
@@ -1,57 +1,48 @@
 set testmodule [file normalize tests/modules/testrdb.so]
 
-proc restart_and_wait {} {
-    catch {
-        r debug restart
-    }
-
-    # wait for the server to come back up
-    set retry 50
-    while {$retry} {
-        if {[catch { r ping }]} {
-            after 100
-        } else {
-            break
-        }
-        incr retry -1
-    }
-}
-
 tags "modules" {
-    start_server [list overrides [list loadmodule "$testmodule"]] {
-        test {modules are able to persist types} {
+    test {modules are able to persist types} {
+        start_server [list overrides [list loadmodule "$testmodule"]] {
             r testrdb.set.key key1 value1
             assert_equal "value1" [r testrdb.get.key key1]
             r debug reload
             assert_equal "value1" [r testrdb.get.key key1]
         }
+    }
 
-        test {modules global are lost without aux} {
+    test {modules global are lost without aux} {
+        set server_path [tmpdir "server.module-testrdb"]
+        start_server [list overrides [list loadmodule "$testmodule" "dir" $server_path]] {
             r testrdb.set.before global1
             assert_equal "global1" [r testrdb.get.before]
-            restart_and_wait
+        }
+        start_server [list overrides [list loadmodule "$testmodule" "dir" $server_path]] {
             assert_equal "" [r testrdb.get.before]
         }
     }
 
-    start_server [list overrides [list loadmodule "$testmodule 2"]] {
-        test {modules are able to persist globals before and after} {
+    test {modules are able to persist globals before and after} {
+        set server_path [tmpdir "server.module-testrdb"]
+        start_server [list overrides [list loadmodule "$testmodule 2" "dir" $server_path]] {
             r testrdb.set.before global1
             r testrdb.set.after global2
             assert_equal "global1" [r testrdb.get.before]
             assert_equal "global2" [r testrdb.get.after]
-            restart_and_wait
+        }
+        start_server [list overrides [list loadmodule "$testmodule 2" "dir" $server_path]] {
             assert_equal "global1" [r testrdb.get.before]
             assert_equal "global2" [r testrdb.get.after]
         }
 
     }
 
-    start_server [list overrides [list loadmodule "$testmodule 1"]] {
-        test {modules are able to persist globals just after} {
+    test {modules are able to persist globals just after} {
+        set server_path [tmpdir "server.module-testrdb"]
+        start_server [list overrides [list loadmodule "$testmodule 1" "dir" $server_path]] {
             r testrdb.set.after global2
             assert_equal "global2" [r testrdb.get.after]
-            restart_and_wait
+        }
+        start_server [list overrides [list loadmodule "$testmodule 1" "dir" $server_path]] {
             assert_equal "global2" [r testrdb.get.after]
         }
     }
-- 
cgit v1.2.1