Fix an issue when module decides to unblock a client which is blocked on keys #11832
Conversation
… on keys. Currently When a module is blocked on keys it sets the CLIENT_PENDING_COMMAND flag. However in case the module decides to unblock the client not via the regular flow (eg timeour, key signal or CLIENT UNBLOCK command) it will attempt to reprocess the module command and potentially blocked again. This fix remove the CLIENT_PENDING_COMMAND flag in case blockedForKeys is issued from module context.
| # we should still get unblocked as the command should not reprocess | ||
| wait_for_blocked_clients_count 0 | ||
| $rd close |
There was a problem hiding this comment.
let's $rd read the response, and maybe add some validation that the command was indeed not reprocessed (not sure how to do that, maybe add some counter in the module?)
There was a problem hiding this comment.
let's
$rd readthe response, and maybe add some validation that the command was indeed not reprocessed (not sure how to do that, maybe add some counter in the module?)
The problem is the as @sjpotter indicated here when a blocked module is blocked on keys and is being unblocked from the module, it will trigger the timeout function.
This is not something new AFAIK (comes from here)
and I think this explains why this is probably a non-existing use case.
To yuo suggestion in case I will try to read from the deferred client the test will fail on error (timeout)
There was a problem hiding this comment.
maybe modify the module to return something so we won't timeout.
also maybe a good idea to add a PING after it, and verify we get the PONG (making sure the protocol didn't get messed up)
There was a problem hiding this comment.
so,in my POC I decided to work around that timeout issue by always giving a timeout of 0 and an empty timeout function, i implement timeouts with a normal RM timer. This avoids the issue.
Even without it calling the timeout function on unblock, the reason for this is because if one is replying in a thread, the timeout function could conceivably be called in parallel to the thread and the timeout function cannot fail today. By implementing it separately in the modules own code (via the timer), I have the control i need.
In practice, this goes to what I'd want for blocking on keys anwyays. i.e. I'd like to see RM_BlockClientOnKeys to be deprecated. And instead it would just be a normal blocking command, but able to setup a SignalOnKeys() with a callback as well as an RM Timer with a callback.
so a module would always BlockClient (not distinguishing between cases) and simply have a signal callback it can setup. Much like timer, if it wants it to repeat
|
this doesn't really change the timeout callback being called, but I wasn't expecting that to be changed with this, its implemented how I implemented it for myself, so LGTM. |
Yes - as I mentioned before - this timeout logic existed before the last refactor and I am not sure why it was done this way. |
@guybe7 maybe you can provide an answer to why this was introduced. |
|
to note, I've tested this branch against my code and it works fine. |
i think that it's there for the unlikely scenario that RM_UnblockClient will be called for a module client that is blocked-on-keys (or maybe in case RM_AbortBlock is called) tbh i really think that both RM_UnblockCLient and RM_ABortBlock should never be called for a client blocked by RM_BlockClientOnKeys... IIRC the idea was that in case the module client is blocked on keys, it's Redis' responsibility to unblock it (or at least to call the reply or timeout callback to invoke module code) |
|
@oranagra I do not think we have a blocker here. can we merge this one? |
|
Yes I think we can merge it |
… on keys (redis#11832) Currently (starting at redis#11012) When a module is blocked on keys it sets the CLIENT_PENDING_COMMAND flag. However in case the module decides to unblock the client not via the regular flow (eg timeout, key signal or CLIENT UNBLOCK command) it will attempt to reprocess the module command and potentially blocked again. This fix remove the CLIENT_PENDING_COMMAND flag in case blockedForKeys is issued from module context.
Currently (starting at #11012) When a module is blocked on keys it sets the CLIENT_PENDING_COMMAND flag.
However in case the module decides to unblock the client not via the regular flow (eg timeout, key signal or CLIENT UNBLOCK command) it will attempt to reprocess the module command and potentially blocked again.
This fix remove the CLIENT_PENDING_COMMAND flag in case blockedForKeys is issued from module context.