From 9c7a4b6619a88b3421c9d4e1c61f911db5096f13 Mon Sep 17 00:00:00 2001 From: Ran Shidlansik Date: Mon, 19 Aug 2024 09:50:15 +0300 Subject: [PATCH 1/5] Make use of a single listNode pointer for blocking utility lists This is to address [This comment](https://github.com/valkey-io/valkey/pull/787#issuecomment-2267490603) which was left as part of #787. Signed-off-by: Ran Shidlansik --- src/blocked.c | 10 ++++++---- src/networking.c | 1 - src/server.h | 10 ++++++++-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/blocked.c b/src/blocked.c index 5f3d8bf504..8e1974a703 100644 --- a/src/blocked.c +++ b/src/blocked.c @@ -81,7 +81,7 @@ void initClientBlockingState(client *c) { c->bstate.numreplicas = 0; c->bstate.numlocal = 0; c->bstate.reploffset = 0; - c->bstate.client_waiting_acks_list_node = NULL; + c->bstate.generic_blocked_list_node = NULL; c->bstate.module_blocked_handle = NULL; c->bstate.async_rm_call_handle = NULL; } @@ -194,8 +194,9 @@ void unblockClient(client *c, int queue_for_reprocessing) { if (moduleClientIsBlockedOnKeys(c)) unblockClientWaitingData(c); unblockClientFromModule(c); } else if (c->bstate.btype == BLOCKED_POSTPONE) { - listDelNode(server.postponed_clients, c->postponed_list_node); - c->postponed_list_node = NULL; + serverAssert(c->bstate.postponed_list_node); + listDelNode(server.postponed_clients, c->bstate.postponed_list_node); + c->bstate.postponed_list_node = NULL; } else if (c->bstate.btype == BLOCKED_SHUTDOWN) { /* No special cleanup. */ } else { @@ -613,7 +614,8 @@ void blockPostponeClient(client *c) { c->bstate.timeout = 0; blockClient(c, BLOCKED_POSTPONE); listAddNodeTail(server.postponed_clients, c); - c->postponed_list_node = listLast(server.postponed_clients); + serverAssert(c->bstate.postponed_list_node == NULL); + c->bstate.postponed_list_node = listLast(server.postponed_clients); /* Mark this client to execute its command */ c->flag.pending_command = 1; } diff --git a/src/networking.c b/src/networking.c index 8b69743530..27d81da493 100644 --- a/src/networking.c +++ b/src/networking.c @@ -212,7 +212,6 @@ client *createClient(connection *conn) { c->peerid = NULL; c->sockname = NULL; c->client_list_node = NULL; - c->postponed_list_node = NULL; c->io_read_state = CLIENT_IDLE; c->io_write_state = CLIENT_IDLE; c->nwritten = 0; diff --git a/src/server.h b/src/server.h index 9dd7f8fefa..35ef00611c 100644 --- a/src/server.h +++ b/src/server.h @@ -1010,6 +1010,14 @@ typedef struct blockingState { * is > timeout then the operation timed out. */ int unblock_on_nokey; /* Whether to unblock the client when at least one of the keys is deleted or does not exist anymore */ + union { + listNode *client_waiting_acks_list_node; /* list node in server.clients_waiting_acks list. */ + listNode *postponed_list_node; /* list node within the postponed list */ + listNode *generic_blocked_list_node; /* generic placeholder for blocked clients utility lists. + Since a client cannot be blocked multiple times, we can assume + It will be held in only one extra utility list, so it is O.K maintain a union of these + listNode references. */ + }; /* BLOCKED_LIST, BLOCKED_ZSET and BLOCKED_STREAM or any other Keys related blocking */ dict *keys; /* The keys we are blocked on */ @@ -1018,7 +1026,6 @@ typedef struct blockingState { int numreplicas; /* Number of replicas we are waiting for ACK. */ int numlocal; /* Indication if WAITAOF is waiting for local fsync. */ long long reploffset; /* Replication offset to reach. */ - listNode *client_waiting_acks_list_node; /* list node in server.clients_waiting_acks list. */ /* BLOCKED_MODULE */ void *module_blocked_handle; /* ValkeyModuleBlockedClient structure. @@ -1321,7 +1328,6 @@ typedef struct client { sds peerid; /* Cached peer ID. */ sds sockname; /* Cached connection target address. */ listNode *client_list_node; /* list node in client list */ - listNode *postponed_list_node; /* list node within the postponed list */ void *module_blocked_client; /* Pointer to the ValkeyModuleBlockedClient associated with this * client. This is set in case of module authentication before the * unblocked client is reprocessed to handle reply callbacks. */ From 27b1c51db8aa09d309f2a9d50bc8428d0e60505f Mon Sep 17 00:00:00 2001 From: Ran Shidlansik Date: Mon, 19 Aug 2024 11:57:46 +0300 Subject: [PATCH 2/5] format fixes Signed-off-by: Ran Shidlansik --- src/server.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/server.h b/src/server.h index 35ef00611c..9454ab2e07 100644 --- a/src/server.h +++ b/src/server.h @@ -1015,17 +1015,17 @@ typedef struct blockingState { listNode *postponed_list_node; /* list node within the postponed list */ listNode *generic_blocked_list_node; /* generic placeholder for blocked clients utility lists. Since a client cannot be blocked multiple times, we can assume - It will be held in only one extra utility list, so it is O.K maintain a union of these - listNode references. */ + it will be held in only one extra utility list, so it is O.K maintain a + union of these listNode references. */ }; /* BLOCKED_LIST, BLOCKED_ZSET and BLOCKED_STREAM or any other Keys related blocking */ dict *keys; /* The keys we are blocked on */ /* BLOCKED_WAIT and BLOCKED_WAITAOF */ - int numreplicas; /* Number of replicas we are waiting for ACK. */ - int numlocal; /* Indication if WAITAOF is waiting for local fsync. */ - long long reploffset; /* Replication offset to reach. */ + int numreplicas; /* Number of replicas we are waiting for ACK. */ + int numlocal; /* Indication if WAITAOF is waiting for local fsync. */ + long long reploffset; /* Replication offset to reach. */ /* BLOCKED_MODULE */ void *module_blocked_handle; /* ValkeyModuleBlockedClient structure. From 1f470f92d8a54651ff06a70ae6c35fc0b5c4a113 Mon Sep 17 00:00:00 2001 From: ranshid <88133677+ranshid@users.noreply.github.com> Date: Mon, 19 Aug 2024 17:32:39 +0300 Subject: [PATCH 3/5] Update src/server.h MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Viktor Söderqvist Signed-off-by: ranshid <88133677+ranshid@users.noreply.github.com> --- src/server.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server.h b/src/server.h index 9454ab2e07..4b157b19eb 100644 --- a/src/server.h +++ b/src/server.h @@ -1012,7 +1012,7 @@ typedef struct blockingState { is deleted or does not exist anymore */ union { listNode *client_waiting_acks_list_node; /* list node in server.clients_waiting_acks list. */ - listNode *postponed_list_node; /* list node within the postponed list */ + listNode *postponed_list_node; /* list node in server.postponed_clients */ listNode *generic_blocked_list_node; /* generic placeholder for blocked clients utility lists. Since a client cannot be blocked multiple times, we can assume it will be held in only one extra utility list, so it is O.K maintain a From 5fcc6d8a9685dd0861b67504020e6d7a0f7c37c3 Mon Sep 17 00:00:00 2001 From: ranshid <88133677+ranshid@users.noreply.github.com> Date: Tue, 20 Aug 2024 12:21:59 +0300 Subject: [PATCH 4/5] Update src/server.h Signed-off-by: ranshid <88133677+ranshid@users.noreply.github.com> --- src/server.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server.h b/src/server.h index 4b157b19eb..609c12b8b6 100644 --- a/src/server.h +++ b/src/server.h @@ -1015,7 +1015,7 @@ typedef struct blockingState { listNode *postponed_list_node; /* list node in server.postponed_clients */ listNode *generic_blocked_list_node; /* generic placeholder for blocked clients utility lists. Since a client cannot be blocked multiple times, we can assume - it will be held in only one extra utility list, so it is O.K maintain a + it will be held in only one extra utility list, so it is ok to maintain a union of these listNode references. */ }; From 0bfdf50bfd0f2309ceb24a1f1592d73d8ca702a5 Mon Sep 17 00:00:00 2001 From: Binbin Date: Tue, 20 Aug 2024 18:21:34 +0800 Subject: [PATCH 5/5] Fix odd format warning Signed-off-by: Binbin --- src/server.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server.h b/src/server.h index 609c12b8b6..7fb7b2a4e5 100644 --- a/src/server.h +++ b/src/server.h @@ -1015,8 +1015,8 @@ typedef struct blockingState { listNode *postponed_list_node; /* list node in server.postponed_clients */ listNode *generic_blocked_list_node; /* generic placeholder for blocked clients utility lists. Since a client cannot be blocked multiple times, we can assume - it will be held in only one extra utility list, so it is ok to maintain a - union of these listNode references. */ + it will be held in only one extra utility list, so it is ok to maintain + a union of these listNode references. */ }; /* BLOCKED_LIST, BLOCKED_ZSET and BLOCKED_STREAM or any other Keys related blocking */