Skip to content

Conversation

@soloestoy
Copy link
Contributor

Every time when accept a connection, we add the client to server.clients list's tail, but in clientsCron we rotate the tail to head at first, and then process the head. It means that the "new" client would be processed before "old" client, moreover if connections established and then freed frequently, the "old" client may have no chance to be processed.

To fix it, we need take a fair way to iterate the list, that is take the current head and process, and then rotate the head to tail, thus we can make sure all clients could be processed step by step.

p.s. client has client_list_node pointer, we don't need put the current client to head to avoid O(N) when remove it.

Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

I have some vague memory of some other piece of code that depended on that listRotateTailToHead call, possibly doing the counter rotation or putting things on one end due to that rotation, but i can't find that.
i guess that either this concern was already changed / dropped, or that i'm just mixing things up.
either way, this change seems safe to me.

@soloestoy
Copy link
Contributor Author

I have some vague memory of some other piece of code that depended on that listRotateTailToHead call

I'm also worried about it, but I didn't find that code, we can merge it or wait more feedbacks?

@oranagra
Copy link
Member

i think we can merge it.. we both reviewed this specific concern and didn't find any problems.

@soloestoy soloestoy added the release-notes indication that this issue needs to be mentioned in the release notes label Apr 24, 2023
@soloestoy soloestoy merged commit bedecec into redis:unstable Apr 24, 2023
@oranagra oranagra mentioned this pull request May 15, 2023
@enjoy-binbin
Copy link
Contributor

i see in putClientInPendingWriteQueue, we always add the node (new client) to the head

listLinkNodeHead(server.clients_pending_write, &c->clients_pending_write_node);

and in handleClientsWithPendingWrites, we process the list from head to tail

    listRewind(server.clients_pending_write,&li);
    while((ln = listNext(&li))) {
        client *c = listNodeValue(ln);
    ...
        /* Try to write buffers to the client socket. */
        if (writeToClient(c,0) == C_ERR) continue;

        /* If after the synchronous writes above we still have data to
         * output to the client, we need to install the writable handler. */
        if (clientHasPendingReplies(c)) {
            installClientWriteHandler(c);
        }

will the order here cause problems? the new clients will be processed before old clients.
maybe it's not a problem, posted here since this seems somewhat relevant

@oranagra
Copy link
Member

why does it matter at which order we handle pending writes?

@enjoy-binbin
Copy link
Contributor

new clients will be processed earlier than old clients, it is not FIFO.
anyway it doesn't seem to be a problem

@soloestoy
Copy link
Contributor Author

server.clients_pending_write is different from server.clients. When handleClientsWithPendingWrites processes server.clients_pending_write, it removes the client from the list without reinserting it, so the order is not important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants