Fix messed-up unblocked clients in flush command #13865
Conversation
|
|
||
| /* FLUSH command is finished. resetClient() and update replication offset. */ | ||
| commandProcessed(c); | ||
| if (c->flags & CLIENT_PENDING_COMMAND) { |
There was a problem hiding this comment.
this, combined with the check in unblockClient, would mean that resetClient will be performed only once, right?
but maybe the call to resetClient and reqresAppendResponse should be converted to a call for commandProcessed?
on one hand, the command was aborted in that flow, but on the other hand, it was replied, so in that way it was processed.
There was a problem hiding this comment.
Do you mean change the resetClient and reqresAppendResponse calls in unblock to call commandProcessed()?
This is a bit strange, if there is no CLIENT_PENDING_COMMAND flag, we will ignore the current execution command.
If calling commandProcessed in unblock is a bit confusing, we don't actually reprocess this command, we don't even do any cleaning up.
Another point is that commandProcessed() is prohibited from being called by a blocked client, and we will have to move if (!(c->flags & CLIENT_PENDING_COMMAND) && c->bstate.btype != BLOCKED_SHUTDOWN) down, not sure if there is a risk.
There was a problem hiding this comment.
ok, let's drop it.
i didn't look too much into it. just saw two places doing resetClient + reqresAppendResponse, and thought it's dangerous since we can add / change one, and forget the other.
There was a problem hiding this comment.
If we need to process the previous command after unblock, we should do so as follows:
unblockClient();
if (c->flags & CLIENT_PENDING_COMMAND) {
c->flags &= ~CLIENT_PENDING_COMMAND;
.... /* continue process command */
}| elapsedStart(&c->bstate.lazyfreeStartTime); | ||
|
|
||
| c->bstate.timeout = 0; | ||
| c->flags |= CLIENT_PENDING_COMMAND; |
There was a problem hiding this comment.
are you saying that we were missing n update of the replication offset? can you describe the flow?
There was a problem hiding this comment.
No, the original flow is fine, I just want to use it to illustrate the following flow that we continue with the previous command, so we can assume that we will continue to process it after unblockclient().
There was a problem hiding this comment.
so this is just a cleanup not to to avoid unblockclient reset a command that is still being handled?
and has no actual implications? maybe you can improve the description..
There was a problem hiding this comment.
Added comment for it.
There was a problem hiding this comment.
i thin i meant in the PR description 😄
There was a problem hiding this comment.
updated the top comment.
There was a problem hiding this comment.
doesn't it still makes it look like there was a bug with the replication offset?
There was a problem hiding this comment.
😏 i think it's okay this time.
|
LGTM |
Fix redis#13853 (review) This PR ensures that the client's current command is not reset by unblockClient(), while still needing to be handled after `unblockclient()`. The FLUSH command still requires reprocessing (update the replication offset) after unblockClient(). Therefore, we mark such blocked clients with the CLIENT_PENDING_COMMAND flag to prevent the command from being reset during unblockClient().
Fix #13853 (review)
This PR ensures that the client's current command is not reset by unblockClient(), while still needing to be handled after
unblockclient().The FLUSH command still requires reprocessing (update the replication offset) after unblockClient(). Therefore, we mark such blocked clients with the CLIENT_PENDING_COMMAND flag to prevent the command from being reset during unblockClient().