From be7a566dbfd892a52440efbc149e240317e871e6 Mon Sep 17 00:00:00 2001 From: Binbin Date: Mon, 23 Sep 2024 11:30:33 +0800 Subject: [PATCH 1/6] Fix module call CLUSTER SLOTS / SHARDS fake client check crash The reason is VM_Call will use a fake client without connection, so we also need to check if c->conn is NULL. Fixes #1054. Signed-off-by: Binbin --- src/networking.c | 2 +- tests/modules/Makefile | 3 +- tests/modules/cluster.c | 51 ++++++++++++++++++++++++++++++++ tests/unit/moduleapi/cluster.tcl | 24 +++++++++++++-- 4 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 tests/modules/cluster.c diff --git a/src/networking.c b/src/networking.c index 44a94087c9..be2435dc83 100644 --- a/src/networking.c +++ b/src/networking.c @@ -3246,7 +3246,7 @@ char *getClientSockname(client *c) { int isClientConnIpV6(client *c) { /* The cached client peer id is on the form "[IPv6]:port" for IPv6 * addresses, so we just check for '[' here. */ - if (c->conn->type == NULL && server.current_client) { + if ((c->conn == NULL || c->conn->type == NULL) && server.current_client) { /* Fake client? Use current client instead. */ c = server.current_client; } diff --git a/tests/modules/Makefile b/tests/modules/Makefile index 9966b8840e..1690b9b627 100644 --- a/tests/modules/Makefile +++ b/tests/modules/Makefile @@ -63,7 +63,8 @@ TEST_MODULES = \ postnotifications.so \ moduleauthtwo.so \ rdbloadsave.so \ - crash.so + crash.so \ + cluster.so .PHONY: all diff --git a/tests/modules/cluster.c b/tests/modules/cluster.c new file mode 100644 index 0000000000..33dbfe9c4a --- /dev/null +++ b/tests/modules/cluster.c @@ -0,0 +1,51 @@ +#include "valkeymodule.h" + +#define UNUSED(x) (void)(x) + +int test_cluster_slots(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { + UNUSED(argv); + + if (argc != 1) return ValkeyModule_WrongArity(ctx); + + ValkeyModuleCallReply *rep = ValkeyModule_Call(ctx, "CLUSTER", "c", "SLOTS"); + if (!rep) { + ValkeyModule_ReplyWithError(ctx, "ERR NULL reply returned"); + } else { + ValkeyModule_ReplyWithCallReply(ctx, rep); + ValkeyModule_FreeCallReply(rep); + } + + return VALKEYMODULE_OK; +} + +int test_cluster_shards(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { + UNUSED(argv); +q + if (argc != 1) return ValkeyModule_WrongArity(ctx); + + ValkeyModuleCallReply *rep = ValkeyModule_Call(ctx, "CLUSTER", "c", "SHARDS"); + if (!rep) { + ValkeyModule_ReplyWithError(ctx, "ERR NULL reply returned"); + } else { + ValkeyModule_ReplyWithCallReply(ctx, rep); + ValkeyModule_FreeCallReply(rep); + } + + return VALKEYMODULE_OK; +} + +int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { + VALKEYMODULE_NOT_USED(argv); + VALKEYMODULE_NOT_USED(argc); + + if (ValkeyModule_Init(ctx, "cluster", 1, VALKEYMODULE_APIVER_1)== VALKEYMODULE_ERR) + return VALKEYMODULE_ERR; + + if (ValkeyModule_CreateCommand(ctx, "test.cluster_slots", test_cluster_slots, "",0,0,0) == VALKEYMODULE_ERR) + return VALKEYMODULE_ERR; + + if (ValkeyModule_CreateCommand(ctx, "test.cluster_shards", test_cluster_shards, "", 0, 0, 0) == VALKEYMODULE_ERR) + return VALKEYMODULE_ERR; + + return VALKEYMODULE_OK; +} diff --git a/tests/unit/moduleapi/cluster.tcl b/tests/unit/moduleapi/cluster.tcl index 5570f980f2..af29cbfe88 100644 --- a/tests/unit/moduleapi/cluster.tcl +++ b/tests/unit/moduleapi/cluster.tcl @@ -219,8 +219,6 @@ start_cluster 2 2 [list config_lines $modules] { } } -} - set testmodule [file normalize tests/modules/basics.so] set modules [list loadmodule $testmodule] start_cluster 3 0 [list config_lines $modules] { @@ -234,3 +232,25 @@ start_cluster 3 0 [list config_lines $modules] { assert_equal {PONG} [$node3 PING] } } + +set testmodule [file normalize tests/modules/cluster.so] +set modules [list loadmodule $testmodule] +start_cluster 3 0 [list config_lines $modules] { + set node1 [srv 0 client] + set node2 [srv -1 client] + set node3 [srv -2 client] + + test "RM_Call with cluster slots" { + assert_equal [lsort [$node1 cluster slots]] [lsort [$node1 test.cluster_slots]] + assert_equal [lsort [$node2 cluster slots]] [lsort [$node2 test.cluster_slots]] + assert_equal [lsort [$node3 cluster slots]] [lsort [$node3 test.cluster_slots]] + } + + test "RM_Call with cluster shards" { + assert_equal [lsort [$node1 cluster shards]] [lsort [$node1 test.cluster_shards]] + assert_equal [lsort [$node2 cluster shards]] [lsort [$node2 test.cluster_shards]] + assert_equal [lsort [$node3 cluster shards]] [lsort [$node3 test.cluster_shards]] + } +} + +} ;# end tag From a903741b28b9003d95fa5ed971010571014ecafb Mon Sep 17 00:00:00 2001 From: Binbin Date: Mon, 23 Sep 2024 11:39:38 +0800 Subject: [PATCH 2/6] cleanup Signed-off-by: Binbin --- tests/modules/cluster.c | 2 +- tests/unit/moduleapi/cluster.tcl | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/modules/cluster.c b/tests/modules/cluster.c index 33dbfe9c4a..20f00bae52 100644 --- a/tests/modules/cluster.c +++ b/tests/modules/cluster.c @@ -41,7 +41,7 @@ int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int arg if (ValkeyModule_Init(ctx, "cluster", 1, VALKEYMODULE_APIVER_1)== VALKEYMODULE_ERR) return VALKEYMODULE_ERR; - if (ValkeyModule_CreateCommand(ctx, "test.cluster_slots", test_cluster_slots, "",0,0,0) == VALKEYMODULE_ERR) + if (ValkeyModule_CreateCommand(ctx, "test.cluster_slots", test_cluster_slots, "", 0, 0, 0) == VALKEYMODULE_ERR) return VALKEYMODULE_ERR; if (ValkeyModule_CreateCommand(ctx, "test.cluster_shards", test_cluster_shards, "", 0, 0, 0) == VALKEYMODULE_ERR) diff --git a/tests/unit/moduleapi/cluster.tcl b/tests/unit/moduleapi/cluster.tcl index af29cbfe88..5e4244d684 100644 --- a/tests/unit/moduleapi/cluster.tcl +++ b/tests/unit/moduleapi/cluster.tcl @@ -240,13 +240,13 @@ start_cluster 3 0 [list config_lines $modules] { set node2 [srv -1 client] set node3 [srv -2 client] - test "RM_Call with cluster slots" { + test "VM_CALL with cluster slots" { assert_equal [lsort [$node1 cluster slots]] [lsort [$node1 test.cluster_slots]] assert_equal [lsort [$node2 cluster slots]] [lsort [$node2 test.cluster_slots]] assert_equal [lsort [$node3 cluster slots]] [lsort [$node3 test.cluster_slots]] } - test "RM_Call with cluster shards" { + test "VM_CALL with cluster shards" { assert_equal [lsort [$node1 cluster shards]] [lsort [$node1 test.cluster_shards]] assert_equal [lsort [$node2 cluster shards]] [lsort [$node2 test.cluster_shards]] assert_equal [lsort [$node3 cluster shards]] [lsort [$node3 test.cluster_shards]] From 901af1a62863c424528e86d56b404ea59dd8b687 Mon Sep 17 00:00:00 2001 From: Binbin Date: Mon, 23 Sep 2024 12:04:19 +0800 Subject: [PATCH 3/6] fix build Signed-off-by: Binbin --- tests/modules/cluster.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/modules/cluster.c b/tests/modules/cluster.c index 20f00bae52..b3b53d5d93 100644 --- a/tests/modules/cluster.c +++ b/tests/modules/cluster.c @@ -20,7 +20,7 @@ int test_cluster_slots(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc int test_cluster_shards(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { UNUSED(argv); -q + if (argc != 1) return ValkeyModule_WrongArity(ctx); ValkeyModuleCallReply *rep = ValkeyModule_Call(ctx, "CLUSTER", "c", "SHARDS"); From 9a51e0f5ff6cc99f34a9a5d3d42aa7ff805a1abf Mon Sep 17 00:00:00 2001 From: Binbin Date: Mon, 23 Sep 2024 14:56:27 +0800 Subject: [PATCH 4/6] fake client attempt Signed-off-by: Binbin --- src/aof.c | 1 + src/eval.c | 1 + src/functions.c | 1 + src/module.c | 5 ++++- src/networking.c | 7 +++++-- src/server.h | 3 ++- tests/integration/aof.tcl | 23 +++++++++++++++++++++++ 7 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/aof.c b/src/aof.c index f48c8bd1bc..e712295127 100644 --- a/src/aof.c +++ b/src/aof.c @@ -1371,6 +1371,7 @@ struct client *createAOFClient(void) { */ c->raw_flag = 0; c->flag.deny_blocking = 1; + c->flag.fake = 1; /* We set the fake client as a replica waiting for the synchronization * so that the server will not try to send replies to this client. */ diff --git a/src/eval.c b/src/eval.c index 580c35bdcc..3b8390af78 100644 --- a/src/eval.c +++ b/src/eval.c @@ -260,6 +260,7 @@ void scriptingInit(int setup) { if (lctx.lua_client == NULL) { lctx.lua_client = createClient(NULL); lctx.lua_client->flag.script = 1; + lctx.lua_client->flag.fake = 1; /* We do not want to allow blocking commands inside Lua */ lctx.lua_client->flag.deny_blocking = 1; diff --git a/src/functions.c b/src/functions.c index 38d0634927..985d8b3e1a 100644 --- a/src/functions.c +++ b/src/functions.c @@ -408,6 +408,7 @@ int functionsRegisterEngine(const char *engine_name, engine *engine) { client *c = createClient(NULL); c->flag.deny_blocking = 1; c->flag.script = 1; + c->flag.fake = 1; engineInfo *ei = zmalloc(sizeof(*ei)); *ei = (engineInfo){ .name = engine_name_sds, diff --git a/src/module.c b/src/module.c index 24cc6a42e7..938ae6ba70 100644 --- a/src/module.c +++ b/src/module.c @@ -656,6 +656,7 @@ client *moduleAllocTempClient(void) { } else { c = createClient(NULL); c->flag.module = 1; + c->flag.fake = 1; c->user = NULL; /* Root user */ } return c; @@ -894,8 +895,10 @@ void moduleCreateContext(ValkeyModuleCtx *out_ctx, ValkeyModule *module, int ctx out_ctx->flags = ctx_flags; if (ctx_flags & VALKEYMODULE_CTX_TEMP_CLIENT) out_ctx->client = moduleAllocTempClient(); - else if (ctx_flags & VALKEYMODULE_CTX_NEW_CLIENT) + else if (ctx_flags & VALKEYMODULE_CTX_NEW_CLIENT) { out_ctx->client = createClient(NULL); + out_ctx->client->flag.fake = 1; + } /* Calculate the initial yield time for long blocked contexts. * in loading we depend on the server hz, but in other cases we also wait diff --git a/src/networking.c b/src/networking.c index be2435dc83..eac1022696 100644 --- a/src/networking.c +++ b/src/networking.c @@ -346,6 +346,7 @@ client *createCachedResponseClient(int resp) { /* Allocating the `conn` allows to prepare the caching client before adding * data to the clients output buffer by `prepareClientToWrite`. */ recording_client->conn = zcalloc(sizeof(connection)); + recording_client->flag.fake = 1; return recording_client; } @@ -3246,8 +3247,10 @@ char *getClientSockname(client *c) { int isClientConnIpV6(client *c) { /* The cached client peer id is on the form "[IPv6]:port" for IPv6 * addresses, so we just check for '[' here. */ - if ((c->conn == NULL || c->conn->type == NULL) && server.current_client) { - /* Fake client? Use current client instead. */ + if (c->flag.fake && server.current_client) { + /* Fake client? Use current client instead. + * Noted that in here we are assuming server.current_client is set + * and real (aof has already violated this in loadSingleAppendOnlyFil). */ c = server.current_client; } return getClientPeerId(c)[0] == '['; diff --git a/src/server.h b/src/server.h index a5cee03055..36668ae31e 100644 --- a/src/server.h +++ b/src/server.h @@ -1242,7 +1242,8 @@ typedef struct ClientFlags { uint64_t dont_cache_primary : 1; /* In some cases we don't want to cache the primary. For example, the replica * knows that it does not need the cache and required a full sync. With this * flag, we won't cache the primary in freeClient. */ - uint64_t reserved : 6; /* Reserved for future use */ + uint64_t fake : 1; /* This is a fake client without a real connection. */ + uint64_t reserved : 5; /* Reserved for future use */ } ClientFlags; typedef struct client { diff --git a/tests/integration/aof.tcl b/tests/integration/aof.tcl index 72fae9915b..33c7c12d4b 100644 --- a/tests/integration/aof.tcl +++ b/tests/integration/aof.tcl @@ -673,3 +673,26 @@ tags {"aof external:skip"} { } } } + +# make sure the test infra won't use SELECT +set old_singledb $::singledb +set ::singledb 1 + +tags {"aof cluster external:skip"} { + test {Test cluster slots / cluster shards in aof won't crash} { + create_aof $aof_dirpath $aof_file { + append_to_aof [formatCommand cluster slots] + append_to_aof [formatCommand cluster shards] + } + + create_aof_manifest $aof_dirpath $aof_manifest_file { + append_to_manifest "file appendonly.aof.1.incr.aof seq 1 type i\n" + } + + start_server_aof [list dir $server_path cluster-enabled yes] { + assert_equal [r ping] {PONG} + } + } +} + +set ::singledb $old_singledb From 8c054637050e031f8900fab359703f2712012f8f Mon Sep 17 00:00:00 2001 From: Binbin Date: Mon, 23 Sep 2024 22:26:33 +0800 Subject: [PATCH 5/6] Adding script test Signed-off-by: Binbin --- tests/unit/cluster/scripting.tcl | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/unit/cluster/scripting.tcl b/tests/unit/cluster/scripting.tcl index 1cf1421079..88e158afc5 100644 --- a/tests/unit/cluster/scripting.tcl +++ b/tests/unit/cluster/scripting.tcl @@ -88,4 +88,12 @@ start_cluster 1 0 {tags {external:skip cluster}} { assert_match {*Can not run script on cluster, 'no-cluster' flag is set*} $e } + + test "Calling cluster slots in scripts is OK" { + assert_equal [lsort [r 0 cluster slots]] [lsort [r 0 eval "return redis.call('cluster', 'slots')" 0]] + } + + test "Calling cluster shards in scripts is OK" { + assert_equal [lsort [r 0 cluster shards]] [lsort [r 0 eval "return redis.call('cluster', 'shards')" 0]] + } } From 18e71a18be5e9aadfe012f3f511042ff98c43607 Mon Sep 17 00:00:00 2001 From: Binbin Date: Wed, 25 Sep 2024 10:57:02 +0800 Subject: [PATCH 6/6] remove tmp files Signed-off-by: Binbin --- cmake-build-debug/CMakeFiles/clion-Debug-log.txt | 1 - 1 file changed, 1 deletion(-) delete mode 100644 cmake-build-debug/CMakeFiles/clion-Debug-log.txt diff --git a/cmake-build-debug/CMakeFiles/clion-Debug-log.txt b/cmake-build-debug/CMakeFiles/clion-Debug-log.txt deleted file mode 100644 index 4e132a2a4f..0000000000 --- a/cmake-build-debug/CMakeFiles/clion-Debug-log.txt +++ /dev/null @@ -1 +0,0 @@ -CMakeLists.txt not found in /Users/zhubinbin/github/valkey Select CMakeLists.txt