From 9483ab0b8ece47eaa0c6b725e87d9348b521a3e3 Mon Sep 17 00:00:00 2001 From: guybe7 Date: Tue, 14 Feb 2023 19:06:30 +0100 Subject: [PATCH] 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 --- tests/modules/blockonkeys.c | 70 ++++++++----- tests/unit/moduleapi/blockonkeys.tcl | 143 +++++---------------------- tests/unit/type/zset.tcl | 10 +- 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; + if (RedisModule_KeyType(key) != REDISMODULE_KEYTYPE_EMPTY) { + /* Key exists */ + if (RedisModule_ModuleTypeGetType(key) != fsltype) { + /* Key is not FSL */ + 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; + } + + *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; + } } - /* 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 */ - RedisModule_CloseKey(key); - *fsl = NULL; - return 1; - } + 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