From bd5fb97fb2824673da9ad5eb9281694752ed5ef0 Mon Sep 17 00:00:00 2001 From: Binbin Date: Mon, 21 Oct 2024 11:48:51 +0800 Subject: [PATCH 1/3] Use fake client flag to replace not conn check The fake client flag was introduced in #1063, we want this to replace all !conn fake client checks. Signed-off-by: Binbin --- src/networking.c | 6 ++++-- src/server.c | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/networking.c b/src/networking.c index c24a95019b..1478948e52 100644 --- a/src/networking.c +++ b/src/networking.c @@ -314,7 +314,9 @@ int prepareClientToWrite(client *c) { * is set. */ if (c->flag.primary && !c->flag.primary_force_reply) return C_ERR; - if (!c->conn) return C_ERR; /* Fake client for AOF loading. */ + /* Skip the fake client, such as the fake client for AOF loading. */ + if (c->flag.fake) return C_ERR; + serverAssert(c->conn); /* Schedule the client to write the output buffers to the socket, unless * it should already be setup to do so (it has already pending data). */ @@ -4437,7 +4439,7 @@ int checkClientOutputBufferLimits(client *c) { * * Returns 1 if client was (flagged) closed. */ int closeClientOnOutputBufferLimitReached(client *c, int async) { - if (!c->conn) return 0; /* It is unsafe to free fake clients. */ + if (c->flag.fake) return 0; /* It is unsafe to free fake clients. */ serverAssert(c->reply_bytes < SIZE_MAX - (1024 * 64)); /* Note that c->reply_bytes is irrelevant for replica clients * (they use the global repl buffers). */ diff --git a/src/server.c b/src/server.c index ab95f84346..a024380bcf 100644 --- a/src/server.c +++ b/src/server.c @@ -894,7 +894,7 @@ void updateClientMemoryUsage(client *c) { } int clientEvictionAllowed(client *c) { - if (server.maxmemory_clients == 0 || c->flag.no_evict || !c->conn) { + if (server.maxmemory_clients == 0 || c->flag.no_evict || c->flag.fake) { return 0; } int type = getClientType(c); From 01e544f16228f01ac859bb70f406f1210aae4f15 Mon Sep 17 00:00:00 2001 From: Binbin Date: Mon, 21 Oct 2024 14:28:46 +0800 Subject: [PATCH 2/3] adding assert Signed-off-by: Binbin --- src/networking.c | 1 + src/server.c | 1 + 2 files changed, 2 insertions(+) diff --git a/src/networking.c b/src/networking.c index 1478948e52..6226411a12 100644 --- a/src/networking.c +++ b/src/networking.c @@ -4440,6 +4440,7 @@ int checkClientOutputBufferLimits(client *c) { * Returns 1 if client was (flagged) closed. */ int closeClientOnOutputBufferLimitReached(client *c, int async) { if (c->flag.fake) return 0; /* It is unsafe to free fake clients. */ + serverAssert(c->conn); serverAssert(c->reply_bytes < SIZE_MAX - (1024 * 64)); /* Note that c->reply_bytes is irrelevant for replica clients * (they use the global repl buffers). */ diff --git a/src/server.c b/src/server.c index a024380bcf..42418ea657 100644 --- a/src/server.c +++ b/src/server.c @@ -897,6 +897,7 @@ int clientEvictionAllowed(client *c) { if (server.maxmemory_clients == 0 || c->flag.no_evict || c->flag.fake) { return 0; } + serverAssert(c->conn); int type = getClientType(c); return (type == CLIENT_TYPE_NORMAL || type == CLIENT_TYPE_PUBSUB); } From 8cbc7d3b84672faad7b32b453dff838bac76c59b Mon Sep 17 00:00:00 2001 From: Binbin Date: Sun, 27 Oct 2024 18:00:34 +0800 Subject: [PATCH 3/3] tmp fix Signed-off-by: Binbin --- src/module.c | 1 + src/networking.c | 9 +++++++-- src/server.h | 7 ++++--- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/module.c b/src/module.c index 2884239200..51105276c6 100644 --- a/src/module.c +++ b/src/module.c @@ -681,6 +681,7 @@ void moduleReleaseTempClient(client *c) { c->bufpos = 0; c->raw_flag = 0; c->flag.module = 1; + c->flag.fake = 1; c->user = NULL; /* Root user */ c->cmd = c->lastcmd = c->realcmd = c->io_parsed_cmd = NULL; if (c->bstate.async_rm_call_handle) { diff --git a/src/networking.c b/src/networking.c index 6226411a12..28bbfeca4c 100644 --- a/src/networking.c +++ b/src/networking.c @@ -314,8 +314,10 @@ int prepareClientToWrite(client *c) { * is set. */ if (c->flag.primary && !c->flag.primary_force_reply) return C_ERR; - /* Skip the fake client, such as the fake client for AOF loading. */ - if (c->flag.fake) return C_ERR; + /* Skip the fake client, such as the fake client for AOF loading. + * But CLIENT_ID_CACHED_RESPONSE is allowed since it is a fake client + * but has a connection to cache the response. */ + if (c->flag.fake && c->id != CLIENT_ID_CACHED_RESPONSE) return C_ERR; serverAssert(c->conn); /* Schedule the client to write the output buffers to the socket, unless @@ -350,6 +352,9 @@ sds aggregateClientOutputBuffer(client *c) { * It needs be paired with `deleteCachedResponseClient` function to stop caching. */ client *createCachedResponseClient(int resp) { struct client *recording_client = createClient(NULL); + /* It is a fake client but with a connection, setting a special client id, + * so we can identify it's a fake cached response client. */ + recording_client->id = CLIENT_ID_CACHED_RESPONSE; recording_client->resp = resp; /* Allocating the `conn` allows to prepare the caching client before adding * data to the clients output buffer by `prepareClientToWrite`. */ diff --git a/src/server.h b/src/server.h index 4fad8d2508..ad0661bec7 100644 --- a/src/server.h +++ b/src/server.h @@ -1084,9 +1084,10 @@ typedef struct { /* With multiplexing we need to take per-client state. * Clients are taken in a linked list. */ -#define CLIENT_ID_AOF (UINT64_MAX) /* Reserved ID for the AOF client. If you \ - need more reserved IDs use UINT64_MAX-1, \ - -2, ... and so forth. */ +#define CLIENT_ID_AOF (UINT64_MAX) /* Reserved ID for the AOF client. If you \ + need more reserved IDs use UINT64_MAX-1, \ + -2, ... and so forth. */ +#define CLIENT_ID_CACHED_RESPONSE (UINT64_MAX - 1) /* Client for cached response, see createCachedResponseClient. */ /* Replication backlog is not a separate memory, it just is one consumer of * the global replication buffer. This structure records the reference of