Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use fake client flag to replace not conn check #1198

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from
Open

Use fake client flag to replace not conn check #1198

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd5fb97
Use fake client flag to replace not conn check
enjoy-binbin Oct 21, 2024
01e544f
adding assert
enjoy-binbin Oct 21, 2024
8cbc7d3
tmp fix
enjoy-binbin Oct 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/networking.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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). */
Expand Down Expand Up @@ -4437,7 +4439,8 @@ 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. */
enjoy-binbin marked this conversation as resolved.
Show resolved Hide resolved
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). */
Expand Down
3 changes: 2 additions & 1 deletion src/server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,9 +894,10 @@ 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;
}
serverAssert(c->conn);
int type = getClientType(c);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serverAssert on c->conn?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good practice. But how do we ensure a new dev/reviewer ensure(s) this? Wouldn't this be better asserted while setting/unsetting the flag in one place? WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be better asserted while setting/unsetting the flag in one place?

I think we would need to create a wrapper like the below and make flag.fake a client internal state.

connection *clientGetConnection(client *c) {
    serverAssert((c->flag.fake && !c->conn) || (!c->flag.fake && c->conn));
    return c->conn;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the wrapper changed will require a lot of code changes, there a LOT OF code are using c->conn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@enjoy-binbin We merge this and file a new issue with help-wanted/good-first-issue tag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not worried about not doing it myself, i just wonder if it's worth doing (diff causes conflicts).

But looking at the code again, i guess doing this wrapper is indeed a good way to avoid fake client connection issue in the future. I will do it in a later commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to feel like we might still need to keep c->conn...

return (type == CLIENT_TYPE_NORMAL || type == CLIENT_TYPE_PUBSUB);
}
Expand Down
Loading