From c85a9b7896b9727709b71fed414bf8a117436d71 Mon Sep 17 00:00:00 2001 From: Binbin Date: Tue, 12 Dec 2023 02:15:19 +0800 Subject: [PATCH] Fix delKeysInSlot server events are not executed inside an execution unit (#12745) This is a follow-up fix to #12733. We need to apply the same changes to delKeysInSlot. Refer to #12733 for more details. This PR contains some other minor cleanups / improvements to the test suite and docs. It uses the postnotifications test module in a cluster mode test which revealed a leak in the test module (fixed). --- redis.conf | 1 + src/cluster_legacy.c | 2 ++ tests/modules/postnotifications.c | 15 ++++++++++----- tests/support/server.tcl | 4 ++-- tests/unit/moduleapi/cluster.tcl | 10 +++++++--- 5 files changed, 22 insertions(+), 10 deletions(-) diff --git a/redis.conf b/redis.conf index 5296a6fa5..c7499ce1f 100644 --- a/redis.conf +++ b/redis.conf @@ -51,6 +51,7 @@ # # loadmodule /path/to/my_module.so # loadmodule /path/to/other_module.so +# loadmodule /path/to/args_module.so [arg [arg ...]] ################################## NETWORK ##################################### diff --git a/src/cluster_legacy.c b/src/cluster_legacy.c index 340b2dfe9..801becf3e 100644 --- a/src/cluster_legacy.c +++ b/src/cluster_legacy.c @@ -5698,6 +5698,7 @@ unsigned int delKeysInSlot(unsigned int hashslot) { dictEntry *de = NULL; iter = dictGetSafeIterator(server.db->dict[hashslot]); while((de = dictNext(iter)) != NULL) { + enterExecutionUnit(1, 0); sds sdskey = dictGetKey(de); robj *key = createStringObject(sdskey, sdslen(sdskey)); dbDelete(&server.db[0], key); @@ -5707,6 +5708,7 @@ unsigned int delKeysInSlot(unsigned int hashslot) { * The modules needs to know that these keys are no longer available locally, so just send the * keyspace notification to the modules, but not to clients. */ moduleNotifyKeyspaceEvent(NOTIFY_GENERIC, "del", key, server.db[0].id); + exitExecutionUnit(); postExecutionUnitOperations(); decrRefCount(key); j++; diff --git a/tests/modules/postnotifications.c b/tests/modules/postnotifications.c index e40cf2994..770711bc3 100644 --- a/tests/modules/postnotifications.c +++ b/tests/modules/postnotifications.c @@ -75,7 +75,8 @@ static int KeySpace_NotificationExpired(RedisModuleCtx *ctx, int type, const cha REDISMODULE_NOT_USED(key); RedisModuleString *new_key = RedisModule_CreateString(NULL, "expired", 7); - RedisModule_AddPostNotificationJob(ctx, KeySpace_PostNotificationString, new_key, KeySpace_PostNotificationStringFreePD); + int res = RedisModule_AddPostNotificationJob(ctx, KeySpace_PostNotificationString, new_key, KeySpace_PostNotificationStringFreePD); + if (res == REDISMODULE_ERR) KeySpace_PostNotificationStringFreePD(new_key); return REDISMODULE_OK; } @@ -95,7 +96,8 @@ static int KeySpace_NotificationEvicted(RedisModuleCtx *ctx, int type, const cha } RedisModuleString *new_key = RedisModule_CreateString(NULL, "evicted", 7); - RedisModule_AddPostNotificationJob(ctx, KeySpace_PostNotificationString, new_key, KeySpace_PostNotificationStringFreePD); + int res = RedisModule_AddPostNotificationJob(ctx, KeySpace_PostNotificationString, new_key, KeySpace_PostNotificationStringFreePD); + if (res == REDISMODULE_ERR) KeySpace_PostNotificationStringFreePD(new_key); return REDISMODULE_OK; } @@ -121,7 +123,8 @@ static int KeySpace_NotificationString(RedisModuleCtx *ctx, int type, const char new_key = RedisModule_CreateStringPrintf(NULL, "string_changed{%s}", key_str); } - RedisModule_AddPostNotificationJob(ctx, KeySpace_PostNotificationString, new_key, KeySpace_PostNotificationStringFreePD); + int res = RedisModule_AddPostNotificationJob(ctx, KeySpace_PostNotificationString, new_key, KeySpace_PostNotificationStringFreePD); + if (res == REDISMODULE_ERR) KeySpace_PostNotificationStringFreePD(new_key); return REDISMODULE_OK; } @@ -137,7 +140,8 @@ static int KeySpace_LazyExpireInsidePostNotificationJob(RedisModuleCtx *ctx, int } RedisModuleString *new_key = RedisModule_CreateString(NULL, key_str + 5, strlen(key_str) - 5);; - RedisModule_AddPostNotificationJob(ctx, KeySpace_PostNotificationReadKey, new_key, KeySpace_PostNotificationStringFreePD); + int res = RedisModule_AddPostNotificationJob(ctx, KeySpace_PostNotificationReadKey, new_key, KeySpace_PostNotificationStringFreePD); + if (res == REDISMODULE_ERR) KeySpace_PostNotificationStringFreePD(new_key); return REDISMODULE_OK; } @@ -236,7 +240,8 @@ static void KeySpace_ServerEventCallback(RedisModuleCtx *ctx, RedisModuleEvent e KeySpace_EventPostNotificationCtx *pn_ctx = RedisModule_Alloc(sizeof(*pn_ctx)); pn_ctx->triggered_on = RedisModule_HoldString(NULL, (RedisModuleString*)key_name); pn_ctx->new_key = RedisModule_CreateString(NULL, events[subevent], strlen(events[subevent])); - RedisModule_AddPostNotificationJob(ctx, KeySpace_ServerEventPostNotification, pn_ctx, KeySpace_ServerEventPostNotificationFree); + int res = RedisModule_AddPostNotificationJob(ctx, KeySpace_ServerEventPostNotification, pn_ctx, KeySpace_ServerEventPostNotificationFree); + if (res == REDISMODULE_ERR) KeySpace_ServerEventPostNotificationFree(pn_ctx); } /* This function must be present on each Redis module. It is used in order to diff --git a/tests/support/server.tcl b/tests/support/server.tcl index 018c659e9..8f5659d9b 100644 --- a/tests/support/server.tcl +++ b/tests/support/server.tcl @@ -5,9 +5,9 @@ set ::valgrind_errors {} proc start_server_error {config_file error} { set err {} append err "Can't start the Redis server\n" - append err "CONFIGURATION:" + append err "CONFIGURATION:\n" append err [exec cat $config_file] - append err "\nERROR:" + append err "\nERROR:\n" append err [string trim $error] send_data_packet $::test_server_fd err $err } diff --git a/tests/unit/moduleapi/cluster.tcl b/tests/unit/moduleapi/cluster.tcl index 807508387..cddd90f75 100644 --- a/tests/unit/moduleapi/cluster.tcl +++ b/tests/unit/moduleapi/cluster.tcl @@ -163,21 +163,23 @@ start_cluster 3 0 [list config_lines $modules] { $node2_rd close } -set modules [list loadmodule [file normalize tests/modules/keyspace_events.so]] +set testmodule_keyspace_events [file normalize tests/modules/keyspace_events.so] +set testmodule_postnotifications "[file normalize tests/modules/postnotifications.so] with_key_events" +set modules [list loadmodule $testmodule_keyspace_events loadmodule $testmodule_postnotifications] start_cluster 2 2 [list config_lines $modules] { set master1 [srv 0 client] set master2 [srv -1 client] set replica1 [srv -2 client] set replica2 [srv -3 client] - + test "Verify keys deletion and notification effects happened on cluster slots change are replicated inside multi exec" { $master2 set count_dels_{4oi} 1 $master2 del count_dels_{4oi} assert_equal 1 [$master2 keyspace.get_dels] assert_equal 1 [$replica2 keyspace.get_dels] $master2 set count_dels_{4oi} 1 - + set repl [attach_to_replication_stream_on_connection -3] $master1 cluster bumpepoch @@ -195,10 +197,12 @@ start_cluster 2 2 [list config_lines $modules] { fail "replica did not increase del counter" } + # the {lpush before_deleted count_dels_{4oi}} is a post notification job registered when 'count_dels_{4oi}' was removed assert_replication_stream $repl { {multi} {del count_dels_{4oi}} {keyspace.incr_dels} + {lpush before_deleted count_dels_{4oi}} {exec} } close_replication_stream $repl