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
This commit is contained in:
guybe7 2023-02-14 19:06:30 +01:00 committed by GitHub
parent fd82bccd0e
commit 9483ab0b8e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 74 additions and 149 deletions

View File

@ -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]);

View File

@ -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

View File

@ -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