Minor changes around the blockonkeys test module (#11803)

All of the POP commands must not decr length below 0.
So, get_fsl will delete the key if the length is 0 (unless
the caller wished to create if doesn't exist)

Other:
1. Use REDISMODULE_WRITE where needed (POP commands)
2. Use wait_for_blokced_clients in test

Unrelated:
Use quotes instead of curly braces in zset.tcl, for variable expansion

3 files changed, 74 insertions, 149 deletions

diff --git a/tests/modules/blockonkeys.c b/tests/modules/blockonkeys.c
index 9b6c5e60b..c24ebdc2a 100644
--- a/tests/modules/blockonkeys.c
+++ b/tests/modules/blockonkeys.c
@@ -9,6 +9,11 @@
 
 #define LIST_SIZE 1024
 
+/* The FSL (Fixed-Size List) data type is a low-budget imitation of the
+ * native Redis list, in order to test list-like commands implemented
+ * by a module.
+ * Examples: FSL.PUSH, FSL.BPOP, etc. */
+
 typedef struct {
     long long list[LIST_SIZE];
     long long length;
@@ -59,31 +64,41 @@ void fsl_free(void *value) {
 
 /* ========================== helper methods ======================= */
 
+/* Wrapper to the boilerplate code of opening a key, checking its type, etc.
+ * Returns 0 if `keyname` exists in the dataset, but it's of the wrong type (i.e. not FSL) */
 int get_fsl(RedisModuleCtx *ctx, RedisModuleString *keyname, int mode, int create, fsl_t **fsl, int reply_on_failure) {
+    *fsl = NULL;
     RedisModuleKey *key = RedisModule_OpenKey(ctx, keyname, mode);
 
-    int type = RedisModule_KeyType(key);
-    if (type != REDISMODULE_KEYTYPE_EMPTY && RedisModule_ModuleTypeGetType(key) != fsltype) {
-        RedisModule_CloseKey(key);
-        if (reply_on_failure)
-            RedisModule_ReplyWithError(ctx, REDISMODULE_ERRORMSG_WRONGTYPE);
-        RedisModuleCallReply *reply = RedisModule_Call(ctx, "INCR", "c", "fsl_wrong_type");
-        RedisModule_FreeCallReply(reply);
-        return 0;
-    }
-
-    /* Create an empty value object if the key is currently empty. */
-    if (type == REDISMODULE_KEYTYPE_EMPTY) {
-        if (!create) {
-            /* Key is empty but we cannot create */
+    if (RedisModule_KeyType(key) != REDISMODULE_KEYTYPE_EMPTY) {
+        /* Key exists */
+        if (RedisModule_ModuleTypeGetType(key) != fsltype) {
+            /* Key is not FSL */
             RedisModule_CloseKey(key);
-            *fsl = NULL;
-            return 1;
+            if (reply_on_failure)
+                RedisModule_ReplyWithError(ctx, REDISMODULE_ERRORMSG_WRONGTYPE);
+            RedisModuleCallReply *reply = RedisModule_Call(ctx, "INCR", "c", "fsl_wrong_type");
+            RedisModule_FreeCallReply(reply);
+            return 0;
+        }
+
+        *fsl = RedisModule_ModuleTypeGetValue(key);
+        if (*fsl && !(*fsl)->length && mode & REDISMODULE_WRITE) {
+            /* Key exists, but it's logically empty */
+            if (create) {
+                create = 0; /* No need to create, key exists in its basic state */
+            } else {
+                RedisModule_DeleteKey(key);
+            }
+        } else {
+            /* Key exists, and has elements in it - no need to create anything */
+            create = 0;
         }
+    }
+
+    if (create) {
         *fsl = fsl_type_create();
         RedisModule_ModuleTypeSetValue(key, fsltype, *fsl);
-    } else {
-        *fsl = RedisModule_ModuleTypeGetValue(key);
     }
 
     RedisModule_CloseKey(key);
@@ -124,9 +139,10 @@ int bpop_reply_callback(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
     RedisModuleString *keyname = RedisModule_GetBlockedClientReadyKey(ctx);
 
     fsl_t *fsl;
-    if (!get_fsl(ctx, keyname, REDISMODULE_READ, 0, &fsl, 0) || !fsl)
+    if (!get_fsl(ctx, keyname, REDISMODULE_WRITE, 0, &fsl, 0) || !fsl)
         return REDISMODULE_ERR;
 
+    RedisModule_Assert(fsl->length);
     RedisModule_ReplyWithLongLong(ctx, fsl->list[--fsl->length]);
     return REDISMODULE_OK;
 }
@@ -155,13 +171,14 @@ int fsl_bpop(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
     }
 
     fsl_t *fsl;
-    if (!get_fsl(ctx, argv[1], REDISMODULE_READ, 0, &fsl, 1))
+    if (!get_fsl(ctx, argv[1], REDISMODULE_WRITE, 0, &fsl, 1))
         return REDISMODULE_OK;
 
     if (!fsl) {
         RedisModule_BlockClientOnKeys(ctx, bpop_reply_callback, to_cb ? bpop_timeout_callback : NULL,
                                       NULL, timeout, &argv[1], 1, NULL);
     } else {
+        RedisModule_Assert(fsl->length);
         RedisModule_ReplyWithLongLong(ctx, fsl->list[--fsl->length]);
     }
 
@@ -175,12 +192,13 @@ int bpopgt_reply_callback(RedisModuleCtx *ctx, RedisModuleString **argv, int arg
     long long *pgt = RedisModule_GetBlockedClientPrivateData(ctx);
 
     fsl_t *fsl;
-    if (!get_fsl(ctx, keyname, REDISMODULE_READ, 0, &fsl, 0) || !fsl)
+    if (!get_fsl(ctx, keyname, REDISMODULE_WRITE, 0, &fsl, 0) || !fsl)
         return RedisModule_ReplyWithError(ctx,"UNBLOCKED key no longer exists");
 
     if (fsl->list[fsl->length-1] <= *pgt)
         return REDISMODULE_ERR;
 
+    RedisModule_Assert(fsl->length);
     RedisModule_ReplyWithLongLong(ctx, fsl->list[--fsl->length]);
     return REDISMODULE_OK;
 }
@@ -211,7 +229,7 @@ int fsl_bpopgt(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
         return RedisModule_ReplyWithError(ctx,"ERR invalid timeout");
 
     fsl_t *fsl;
-    if (!get_fsl(ctx, argv[1], REDISMODULE_READ, 0, &fsl, 1))
+    if (!get_fsl(ctx, argv[1], REDISMODULE_WRITE, 0, &fsl, 1))
         return REDISMODULE_OK;
 
     if (!fsl)
@@ -226,6 +244,7 @@ int fsl_bpopgt(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
             bpopgt_free_privdata, timeout, &argv[1], 1, pgt,
             REDISMODULE_BLOCK_UNBLOCK_DELETED);
     } else {
+        RedisModule_Assert(fsl->length);
         RedisModule_ReplyWithLongLong(ctx, fsl->list[--fsl->length]);
     }
 
@@ -239,13 +258,14 @@ int bpoppush_reply_callback(RedisModuleCtx *ctx, RedisModuleString **argv, int a
     RedisModuleString *dst_keyname = RedisModule_GetBlockedClientPrivateData(ctx);
 
     fsl_t *src;
-    if (!get_fsl(ctx, src_keyname, REDISMODULE_READ, 0, &src, 0) || !src)
+    if (!get_fsl(ctx, src_keyname, REDISMODULE_WRITE, 0, &src, 0) || !src)
         return REDISMODULE_ERR;
 
     fsl_t *dst;
     if (!get_fsl(ctx, dst_keyname, REDISMODULE_WRITE, 1, &dst, 0) || !dst)
         return REDISMODULE_ERR;
 
+    RedisModule_Assert(src->length);
     long long ele = src->list[--src->length];
     dst->list[dst->length++] = ele;
     RedisModule_SignalKeyAsReady(ctx, dst_keyname);
@@ -274,7 +294,7 @@ int fsl_bpoppush(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
         return RedisModule_ReplyWithError(ctx,"ERR invalid timeout");
 
     fsl_t *src;
-    if (!get_fsl(ctx, argv[1], REDISMODULE_READ, 0, &src, 1))
+    if (!get_fsl(ctx, argv[1], REDISMODULE_WRITE, 0, &src, 1))
         return REDISMODULE_OK;
 
     if (!src) {
@@ -287,6 +307,8 @@ int fsl_bpoppush(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
         fsl_t *dst;
         if (!get_fsl(ctx, argv[2], REDISMODULE_WRITE, 1, &dst, 1))
             return REDISMODULE_OK;
+
+        RedisModule_Assert(src->length);
         long long ele = src->list[--src->length];
         dst->list[dst->length++] = ele;
         RedisModule_SignalKeyAsReady(ctx, argv[2]);
diff --git a/tests/unit/moduleapi/blockonkeys.tcl b/tests/unit/moduleapi/blockonkeys.tcl
index 50b130ba2..85faa7525 100644
--- a/tests/unit/moduleapi/blockonkeys.tcl
+++ b/tests/unit/moduleapi/blockonkeys.tcl
@@ -10,15 +10,13 @@ start_server {tags {"modules"}} {
         r del src dst
 
         $rd1 fsl.bpoppush src dst 0
+        wait_for_blocked_clients_count 1
+
         $rd2 fsl.bpoppush dst src 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {2}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 2
 
         r fsl.push src 42
+        wait_for_blocked_clients_count 0
 
         assert_equal {42} [r fsl.getall src]
         assert_equal {} [r fsl.getall dst]
@@ -30,12 +28,7 @@ start_server {tags {"modules"}} {
         r del src
 
         $rd1 fsl.bpoppush src src 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r fsl.push src 42
 
         assert_equal {42} [r fsl.getall src]
@@ -59,12 +52,7 @@ start_server {tags {"modules"}} {
         r del k
         set rd [redis_deferring_client]
         $rd fsl.bpop k 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r fsl.push k 34
         assert_equal {34} [$rd read]
     }
@@ -93,12 +81,7 @@ start_server {tags {"modules"}} {
         set cid [$rd read]
         r fsl.push k 33
         $rd fsl.bpopgt k 33 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r fsl.push k 34
         assert_equal {34} [$rd read]
         r client kill id $cid ;# try to smoke-out client-related memory leak
@@ -109,12 +92,7 @@ start_server {tags {"modules"}} {
         r fsl.push k 32
         set rd [redis_deferring_client]
         $rd fsl.bpopgt k 35 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r fsl.push k 33
         r fsl.push k 34
         r fsl.push k 35
@@ -127,12 +105,7 @@ start_server {tags {"modules"}} {
         r fsl.push k 32
         set rd [redis_deferring_client]
         $rd fsl.bpopgt k 35 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r del k
         assert_error {*UNBLOCKED key no longer exists*} {$rd read}
     }
@@ -142,12 +115,7 @@ start_server {tags {"modules"}} {
         r fsl.push k 32
         set rd [redis_deferring_client]
         $rd fsl.bpopgt k 35 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r flushall
         assert_error {*UNBLOCKED key no longer exists*} {$rd read}
     }
@@ -158,12 +126,7 @@ start_server {tags {"modules"}} {
         r fsl.push k 32
         set rd [redis_deferring_client]
         $rd fsl.bpopgt k 35 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r swapdb 0 9
         assert_error {*UNBLOCKED key no longer exists*} {$rd read}
     }
@@ -178,12 +141,7 @@ start_server {tags {"modules"}} {
         r select 9
         set rd [redis_deferring_client]
         $rd fsl.bpopgt k 35 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r swapdb 0 9
         assert_error {*UNBLOCKED key no longer exists*} {$rd read}
         r select 9
@@ -199,12 +157,7 @@ start_server {tags {"modules"}} {
         r select 9
         set rd [redis_deferring_client]
         $rd fsl.bpopgt k 35 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r swapdb 0 9
         assert_equal {1} [s 0 blocked_clients]
         r fsl.push k 38
@@ -222,12 +175,7 @@ start_server {tags {"modules"}} {
         r select 9
         set rd [redis_deferring_client]
         $rd fsl.bpopgt k 35 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r swapdb 0 9
         assert_equal {38} [$rd read]
         r select 9
@@ -240,12 +188,7 @@ start_server {tags {"modules"}} {
         $rd client id
         set cid [$rd read]
         $rd fsl.bpopgt k 35 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r client kill id $cid ;# try to smoke-out client-related memory leak
     }
 
@@ -256,12 +199,7 @@ start_server {tags {"modules"}} {
         $rd client id
         set cid [$rd read]
         $rd fsl.bpopgt k 35 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r client unblock $cid timeout ;# try to smoke-out client-related memory leak
         assert_equal {Request timedout} [$rd read]
     }
@@ -273,12 +211,7 @@ start_server {tags {"modules"}} {
         $rd client id
         set cid [$rd read]
         $rd fsl.bpopgt k 35 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r client unblock $cid error ;# try to smoke-out client-related memory leak
         assert_error "*unblocked*" {$rd read}
     }
@@ -289,12 +222,7 @@ start_server {tags {"modules"}} {
         $rd client id
         set cid [$rd read]
         $rd fsl.bpop k 0 NO_TO_CB
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         assert_equal [r client unblock $cid timeout] {0}
         $rd close
     }
@@ -305,12 +233,7 @@ start_server {tags {"modules"}} {
         $rd client id
         set cid [$rd read]
         $rd fsl.bpop k 0 NO_TO_CB
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         assert_equal [r client unblock $cid error] {0}
         $rd close
     }
@@ -319,12 +242,7 @@ start_server {tags {"modules"}} {
         r del k
         set rd [redis_deferring_client]
         $rd fsl.bpop k 0
-        ;# wait until clients are actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Clients are not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r lpush k 12
         r lpush k 13
         r lpush k 14
@@ -338,12 +256,7 @@ start_server {tags {"modules"}} {
         r del k
         set rd [redis_deferring_client]
         $rd blockonkeys.popall k
-        # wait until client is actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Client is not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r lpush k 42 squirrel banana
         assert_equal {banana squirrel 42} [$rd read]
         $rd close
@@ -353,12 +266,7 @@ start_server {tags {"modules"}} {
         r del k
         set rd [redis_deferring_client]
         $rd blpop k 3
-        # wait until client is actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Client is not blocked"
-        }
+        wait_for_blocked_clients_count 1
         r blockonkeys.lpush k 42
         assert_equal {k 42} [$rd read]
         $rd close
@@ -370,12 +278,7 @@ start_server {tags {"modules"}} {
         # Module client blocks to pop 5 elements from list
         set rd [redis_deferring_client]
         $rd blockonkeys.blpopn k 5
-        # Wait until client is actually blocked
-        wait_for_condition 50 100 {
-            [s 0 blocked_clients] eq {1}
-        } else {
-            fail "Client is not blocked"
-        }
+        wait_for_blocked_clients_count 1
         # Check that RM_SignalKeyAsReady() can wake up BLPOPN
         r blockonkeys.lpush_unblock k bb cc ;# Not enough elements for BLPOPN
         r lpush k dd ee ff                  ;# Doesn't unblock module
diff --git a/tests/unit/type/zset.tcl b/tests/unit/type/zset.tcl
index 036638510..eb5cb8432 100644
--- a/tests/unit/type/zset.tcl
+++ b/tests/unit/type/zset.tcl
@@ -308,29 +308,29 @@ start_server {tags {"zset"}} {
             assert_error "*NaN*" {r zincrby myzset -inf abc}
         }
 
-        test {ZADD - Variadic version base case - $encoding} {
+        test "ZADD - Variadic version base case - $encoding" {
             r del myzset
             list [r zadd myzset 10 a 20 b 30 c] [r zrange myzset 0 -1 withscores]
         } {3 {a 10 b 20 c 30}}
 
-        test {ZADD - Return value is the number of actually added items - $encoding} {
+        test "ZADD - Return value is the number of actually added items - $encoding" {
             list [r zadd myzset 5 x 20 b 30 c] [r zrange myzset 0 -1 withscores]
         } {1 {x 5 a 10 b 20 c 30}}
 
-        test {ZADD - Variadic version does not add nothing on single parsing err - $encoding} {
+        test "ZADD - Variadic version does not add nothing on single parsing err - $encoding" {
             r del myzset
             catch {r zadd myzset 10 a 20 b 30.badscore c} e
             assert_match {*ERR*not*float*} $e
             r exists myzset
         } {0}
 
-        test {ZADD - Variadic version will raise error on missing arg - $encoding} {
+        test "ZADD - Variadic version will raise error on missing arg - $encoding" {
             r del myzset
             catch {r zadd myzset 10 a 20 b 30 c 40} e
             assert_match {*ERR*syntax*} $e
         }
 
-        test {ZINCRBY does not work variadic even if shares ZADD implementation - $encoding} {
+        test "ZINCRBY does not work variadic even if shares ZADD implementation - $encoding" {
             r del myzset
             catch {r zincrby myzset 10 a 20 b 30 c} e
             assert_match {*ERR*wrong*number*arg*} $e