Skip to content

Conversation

@ranshid
Copy link
Contributor

@ranshid ranshid commented Jul 19, 2022

TL;DR

Following the discussion over the issue #7551
We decided to refactor the client blocking code to eliminate some of the code duplications
and to rebuild the infrastructure better for future key blocking cases.

In this PR

  1. reprocess the command once a client becomes unblocked on key (instead of running custom code for the unblocked path that's different than the one that would have run if blocking wasn't needed)
  2. eliminate some (now) irrelevant code for handling unblocking lists/zsets/streams etc...
  3. modify some tests to intercept the error in cases of error on reprocess after unblock (see details in the notes section below)
  4. replace '$' on the client argv with current stream id. Since once we reprocess the stream XREAD we need to read from the last msg and not wait for new msg in order to prevent endless block loop.
  5. Added statistics to the info "Clients" section to report the:
    • total_blocking_keys - number of blocking keys
    • total_blocking_keys_on_nokey - number of blocking keys which have at least 1 client which would like
      to be unblocked on when the key is deleted.
  6. Avoid expiring unblocked key during unblock. Previously we used to lookup the unblocked key which might have been expired during the lookup. Now we lookup the key using NOTOUCH and NOEXPIRE to avoid deleting it at this point, so propagating commands in blocked.c is no longer needed.
  7. deprecated command flags. We decided to remove the CMD_CALL_STATS and CMD_CALL_SLOWLOG and make an explicit verification in the call() function in order to decide if stats update should take place.
    This should simplify the logic and also mitigate existing issues: for example module calls which are triggered as part of AOF loading might still report stats even though they are called during AOF loading.

Behavior changes

  1. As this implementation prevents writing dedicated code handling unblocked streams/lists/zsets,
    since we now re-process the command once the client is unblocked some errors will be reported differently.
    The old implementation used to issue
    UNBLOCKED the stream key no longer exists
    in the following cases:
    • The stream key has been deleted (ie. calling DEL)
    • The stream and group existed but the key type was changed by overriding it (ie. with set command)
    • The key not longer exists after we swapdb with a db which does not contains this key
    • After swapdb when the new db has this key but with different type.

In the new implementation the reported errors will be the same as if the command was processed after effect:
NOGROUP - in case key no longer exists, or WRONGTYPE in case the key was overridden with a different type.

  1. Reprocessing the command means that some checks will be reevaluated once the client is unblocked.
    For example, ACL rules might change since the command originally was executed and will fail once the client is unblocked.
    Another example is OOM condition checks which might enable the command to run and block but fail the command reprocess once the client is unblocked.

  2. One of the changes in this PR is that no command stats are being updated once the command is blocked (all stats will be updated once the client is unblocked). This implies that when we have many clients blocked, users will no longer be able to get that information from the command stats. However the information can still be gathered from the client list. This also means that clients that get killed or timeout while blocked, will not increment the command stats.

Client blocking

the blocking on key will still be triggered the same way as it is done today.
in order to block the current client on list of keys, the call to
blockForKeys will still need to be made which will perform the same as it is today:

  • add the client to the list of blocked clients on each key
  • keep the key with a matching list node (position in the global blocking clients list for that key) in the client private blocking key dict.
  • flag the client with CLIENT_BLOCKED
  • update blocking statistics
  • register the client on the timeout table

Key Unblock

Unblocking a specific key will be triggered (same as today) by calling signalKeyAsReady.
the implementation in that part will stay the same as today - adding the key to the global readyList.
The reason to maintain the readyList (as apposed to iterating over all clients blocked on the specific key)
is in order to keep the signal operation as short as possible, since it is called during the command processing.
The main change is that instead of going through a dedicated code path that operates the blocked command
we will just call processPendingCommandsAndResetClient.

ClientUnblock (keys)

  1. Unblocking clients on keys will be triggered after command is
    processed and during the beforeSleep
  2. the general schema is:
  3. For each key k in the readyList:
For each client *c* which is blocked on *k*:
            in case either:
	          1. *k* exists AND the *k* type matches the current client blocking type
	  	      OR
	          2. *k* exists and *c* is blocked on module command
	    	      OR
	          3. *k* does not exists and *c* was blocked with the flag
	             unblock_on_deleted_key
                 do:
                                  1. remove the client from the list of clients blocked on this key
                                  2. remove the blocking list node from the client blocking key dict
                                  3. remove the client from the timeout list
                                  10. queue the client on the unblocked_clients list
                                  11. *NEW*: call processCommandAndResetClient(c);

NOTE: for module blocked clients we will still call the moduleUnblockClientByHandle
which will queue the client for processing in moduleUnblockedClients list.

Process Unblocked clients

The process of all unblocked clients is done in the beforeSleep and no change is planned in that part.
The general schema will be:
For each client c in server.unblocked_clients:

    * remove client from the server.unblocked_clients
    * set back the client readHandler
    * continue processing the pending command and input buffer.

Some notes regarding the new implementation

  1. Although it was proposed, it is currently difficult to remove the
    read handler from the client while it is blocked.
    The reason is that a blocked client should be unblocked when it is
    disconnected, or we might consume data into void.

  2. While this PR mainly keep the current blocking logic as-is, there
    might be some future additions to the infrastructure that we would
    like to have:

    • allow non-preemptive blocking of client - sometimes we can think
      that a new kind of blocking can be expected to not be preempt. for
      example lets imagine we hold some keys on disk and when a command
      needs to process them it will block until the keys are uploaded.
      in this case we will want the client to not disconnect or be
      unblocked until the process is completed (remove the client read
      handler, prevent client timeout, disable unblock via debug command etc...).
    • allow generic blocking based on command declared keys - we might
      want to add a hook before command processing to check if any of the
      declared keys require the command to block. this way it would be
      easier to add new kinds of key-based blocking mechanisms.

Signed-off-by: Ran Shidlansik ranshid@amazon.com

@ranshid
Copy link
Contributor Author

ranshid commented Jul 19, 2022

@oranagra/@madolson this is following the issue #7551
lots of small changes in this PR, but I think it brings some order onto things.
@oranagra in case the change of bpop --> bstate is problematic I can remake it, but it killed me to look at it :)

@ranshid
Copy link
Contributor Author

ranshid commented Jul 19, 2022

Just to report that a week ago I run this change on the daily CI: https://github.com/ranshid/redis/actions/runs/2656180654
some errors but I am still not sure it is related to this change

@oranagra oranagra linked an issue Jul 19, 2022 that may be closed by this pull request
@oranagra
Copy link
Member

@ranshid thanks (it'll take some time till i'll get to review this).
for the record, i do support renaming bpop, i think it's about time..

@madolson madolson mentioned this pull request Jul 19, 2022
1 task
Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

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

We probably need to add some tests are doing recursive LMOVE commands, which don't seem to exist.

@madolson madolson requested a review from oranagra September 30, 2022 15:06
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.

Thanks Ran. this area needed some cleanup.
in fact, we could rename it to be part of the "sort out the mess" series 😄
FYI: #11310 (not sure which one to merge first)
CC @guybe7

there is a ton of deleted code here, it's impossible to make sure it didn't have any specific logic for blocked command edge cases that's missing from the actual commands.
i just hope we have enough tests to spot these.
CC @enjoy-binbin @guybe7 maybe you can spot something.

*Client blocking*
---------------------------------------

the blocking on key will still be triggered the same way as it is done today.
in order to block the current client on list of keys, the call to
blockForKeys will still need to be made which will perform the same as it is today:

*  add the client to the list of blocked clients on each key
*  keep the key with a matching blocking info (bkinfo) in the client private blocking key dict.
*  flag the client with CLIENT_BLOCKED
*  update blocking statistics
*  register the client on the timeout table

*Key Unblock*
--------------------------------------

Unblocking a specific key will be triggered (same as today) by calling signalKeyAsReady.
the implementation in that part will stay the same as today - adding the key to the global readyList.
The reason to maintain the readyList (as apposed to iterating over all clients blocked on the specific key)
is in order to keep the signal operation as short as possible, since it is called during the command processing.
The main change is that instead of going through a dedicated code path that operates the blocked command
we will just call processPendingCommandsAndResetClient.

*ClientUnblock (keys)*
--------------------------------------

1. Unblocking clients on keys will be triggered after command is
   processed and during the beforeSleep
2. the general schema is:
3. For each key *k* in the readyList:
            For each client *c* which is blocked on *k*:
            in case either:
	    1. *k* exists AND the *k* type matches the current client blocking type
	  	OR
	    2. *k* exists and *c* is blocked on module command
	    	OR
	    3. *k* does not exists and *c* was blocked with the flag
	       unblock_on_deleted_key
                                            1. remove the client from the list of clients blocked on this key
                                            2. remove the bkinfo from the client blocking key dict
                                            3. remove the client from the timeout list
                                            4. queue the client on the unblocked_clients list
                                            5. *NEW*: call processCommandAndResetClient(c);

					    *NOTE:* for module blocked clients we will still call the moduleUnblockClientByHandle
                                            which will queue the client for processing in moduleUnblockedClients list.

*Process Unblocked clients*
-------------------------------------

The process of all unblocked clients is done in the beforeSleep and no change is planned in that part.
The general schema will be:
For each client *c* in server.unblocked_clients:

        * remove client from the server.unblocked_clients
        * set back the client readHandler
        * continue processing the pending command and input buffer.

*Some notes regarding the proposed implementation*
---------------------------------------------------

1.  As this implementation prevents writing dedicated code handling unblocked streams/lists/zsets,
since we now re-process the command once the client is unblocked some errors might be reported differently.
i.e for the xreadgrouop case (https://github.com/redis/redis/blob/unstable/src/blocked.c#L447-L449).

3. Although it was proposed, it is currently difficult to remove the
   read handler from the client while it is blocked.
   The reason is that a blocked client should be unblocked when it is
   disconnected, or we might consume data into void.

3. While this PR mainly keep the current blocking logic as-is, there
   might be some future additions to the infrastracture that we would
   like to have:
   - allow non-preemptive blocking of client - sometimes we can think
     that a new king of blocking can be expected to not be preempt. for
     example lets imaging we hold some keys on disk and when a command
     needs to process them it will block untill the keys are uploaded.
     in this case we will want the client to not disconnect or be
     unblocked untill the process is completed (remove the client read
     handler, prevent client timeout, disable unblock via debug command etc...).
   - allow generic blocking based on command declared keys - we might
     want to add a hook before command processing to check if any of the
     declared keys require the command to block. this way it would be
     easier to add new kinds of key-based blocking mechanisms.

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
 1. remove unneeded license from new blocking header file - redis#11012 (comment)
 2. move blocking type to enum - redis#11012 (comment)
 3. remove unneeded bki allocations and indirections - redis#11012 (comment)
1. rename init blocking state https://github.com/redis/redis/pull/11012/files#r985241581
2. fix comment https://github.com/redis/redis/pull/11012/files#r985236406
3. remove extra dict find https://github.com/redis/redis/pull/11012/files#r985236975
4. align spaces in function call parameters https://github.com/redis/redis/pull/11012/files#r985238113
5. remove line comments https://github.com/redis/redis/pull/11012/files#r985238760
6. remove module deprecated comment https://github.com/redis/redis/pull/11012/files#r985243184
7. add NOTOUCH and NOEXPIRE when fetching key https://github.com/redis/redis/pull/11012/files#r985245129
8. remove unneeded propagate https://github.com/redis/redis/pull/11012/files#r985245623
9. make stream argv change only ion case of blocking redis#11012 (comment)
10. remove uneeded check for no more blocked keys redis#11012 (comment)
@ranshid ranshid force-pushed the oss-client-blocking branch from 5934ea1 to c116025 Compare October 3, 2022 11:52
@ranshid
Copy link
Contributor Author

ranshid commented Oct 3, 2022

@oranagra , @madolson , @enjoy-binbin I forced push in order to rebase my version on top of the updated repo (merge would have made some salad of commits) I was unable to accommodate all comments, but most of them have been fixed.

@oranagra
Copy link
Member

oranagra commented Oct 3, 2022

@ranshid i don't understand why a merge commit would be a problem. i'd prefer you avoid that next time.
anyway, water under the bridge now, so please tell me which commits are new?
or is the above force-push only pushing an rebase, and no actual new content?

@ranshid ranshid closed this Oct 3, 2022
@ranshid
Copy link
Contributor Author

ranshid commented Oct 3, 2022

@ranshid i don't understand why a merge commit would be a problem. i'd prefer you avoid that next time. anyway, water under the bridge now, so please tell me which commits are new? or is the above force-push only pushing an rebase, and no actual new content?

402e4e0
and
c116025

are the new fixes.

@ranshid ranshid reopened this Oct 3, 2022
@guybe7
Copy link
Collaborator

guybe7 commented Oct 5, 2022

@ranshid i've reviewed some of this PR but in order to continue please undo some code moving (db.c, blocked.c, remove blocked.h) so the diff won't be as big

@ranshid
Copy link
Contributor Author

ranshid commented Oct 6, 2022

I have read the new comments and will address all of them.
However due to some work related obligations and the holidays I will only be able to work on it next week.
I am sorry for the delay.

1. revert back the change around scanDatabaseForDeletedStreams
2. refactor blockForKeys to use dictAddRaw
3. reduce diff of changes in blocking.c
4. make blocking initiator decide if client shuld be unblocked on no key
5. use notouch+noexpire when fetching key on unblock
6. eliminate blocked.h
@ranshid
Copy link
Contributor Author

ranshid commented Oct 12, 2022

@ranshid i've reviewed some of this PR but in order to continue please undo some code moving (db.c, blocked.c, remove blocked.h) so the diff won't be as big

@guybe7 I made some effort to make the diff cleaner. tell me what you think

since we removed the sync feed to replication buffer, when a key is
expiered once read during ublock processing, we need to drain the
replication buffer.
1. make better list.tcl test modifications
2. fix small alignment issue
@oranagra
Copy link
Member

@ranshid the other PR was merged, please merge unstable into this one,
p.s. discussed it and conceptually approved it in a core-team meeting.

@guybe7
Copy link
Collaborator

guybe7 commented Feb 27, 2023

@sjpotter i think the way you use RM_BlockClientOnKeys - can you please explain why don't you unblock the client from the reply_callback? IIUC you use this API not to block a client, but rather to know when a certain key is ready... which is not the right way to use it. why don't you use the "new" keyspace notification?

@ranshid does the fix in #11832 only handles the case where one unblocks a blocked-on-key by using RM_UnblcokClient? or does it handle more common use-case like CLIENT UNBLOCK?

@ranshid
Copy link
Contributor Author

ranshid commented Feb 27, 2023

@ranshid does the fix in #11832 only handles the case where one unblocks a blocked-on-key by using RM_UnblcokClient? or does it handle more common use-case like CLIENT UNBLOCK?

AFAIK this only applies to client unblocking by module when blocked on keys.

oranagra pushed a commit that referenced this pull request Mar 8, 2023
… on keys (#11832)

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.
@sjpotter
Copy link
Contributor

sjpotter commented Mar 9, 2023

found another bug with blocking commands in a module, when reprocessing them. #11894

madolson added a commit that referenced this pull request Mar 30, 2023
In #11012, we changed the way command durations were computed to handle the same command being executed multiple times. This commit fixes some misses from that commit.

* Wait commands were not correctly reporting their duration if the timeout was reached.
* Multi/scripts/and modules with RM_Call were not properly resetting the duration between inner calls, leading to them reporting cumulative duration.
* When a blocked client is freed, the call and duration are always discarded.

This commit also adds an assert if the duration is not properly reset, potentially indicating that a report to call statistics was missed. The assert potentially be removed in the future, as it's mainly intended to detect misses in tests.
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jun 12, 2023
For the XREADGROUP BLOCK > scenario, there is an endless loop.
Due to redis#11012, it keep going, reprocess command -> blockForKeys -> reprocess command

The right fix is to avoid an endless loop in handleClientsBlockedOnKey and handleClientsBlockedOnKeys,
looks like there was some attempt in handleClientsBlockedOnKeys but maybe not sufficiently good,
and it looks like using a similar trick in handleClientsBlockedOnKey is complicated.
i.e. stashing the list on the stack and iterating on it after creating a fresh one for future use,
is problematic since the code keeps accessing the global list.

The fix is proposed by oranagra.

Fixes redis#12290

Co-authored-by: Oran Agra <oran@redislabs.com>
oranagra added a commit that referenced this pull request Jun 13, 2023
For the XREADGROUP BLOCK > scenario, there is an endless loop.
Due to #11012, it keep going, reprocess command -> blockForKeys -> reprocess command

The right fix is to avoid an endless loop in handleClientsBlockedOnKey and handleClientsBlockedOnKeys,
looks like there was some attempt in handleClientsBlockedOnKeys but maybe not sufficiently good,
and it looks like using a similar trick in handleClientsBlockedOnKey is complicated.
i.e. stashing the list on the stack and iterating on it after creating a fresh one for future use,
is problematic since the code keeps accessing the global list.

Co-authored-by: Oran Agra <oran@redislabs.com>
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
*TL;DR*
---------------------------------------
Following the discussion over the issue [redis#7551](redis#7551)
We decided to refactor the client blocking code to eliminate some of the code duplications
and to rebuild the infrastructure better for future key blocking cases.


*In this PR*
---------------------------------------
1. reprocess the command once a client becomes unblocked on key (instead of running
   custom code for the unblocked path that's different than the one that would have run if
   blocking wasn't needed)
2. eliminate some (now) irrelevant code for handling unblocking lists/zsets/streams etc...
3. modify some tests to intercept the error in cases of error on reprocess after unblock (see
   details in the notes section below)
4. replace '$' on the client argv with current stream id. Since once we reprocess the stream
   XREAD we need to read from the last msg and not wait for new msg  in order to prevent
   endless block loop. 
5. Added statistics to the info "Clients" section to report the:
   * `total_blocking_keys` - number of blocking keys
   * `total_blocking_keys_on_nokey` - number of blocking keys which have at least 1 client
      which would like
   to be unblocked on when the key is deleted.
6. Avoid expiring unblocked key during unblock. Previously we used to lookup the unblocked key
   which might have been expired during the lookup. Now we lookup the key using NOTOUCH and
   NOEXPIRE to avoid deleting it at this point, so propagating commands in blocked.c is no longer needed.
7. deprecated command flags. We decided to remove the CMD_CALL_STATS and CMD_CALL_SLOWLOG
   and make an explicit verification in the call() function in order to decide if stats update should take place.
   This should simplify the logic and also mitigate existing issues: for example module calls which are
   triggered as part of AOF loading might still report stats even though they are called during AOF loading.

*Behavior changes*
---------------------------------------------------

1. As this implementation prevents writing dedicated code handling unblocked streams/lists/zsets,
since we now re-process the command once the client is unblocked some errors will be reported differently.
The old implementation used to issue
``UNBLOCKED the stream key no longer exists``
in the following cases:
   - The stream key has been deleted (ie. calling DEL)
   - The stream and group existed but the key type was changed by overriding it (ie. with set command)
   - The key not longer exists after we swapdb with a db which does not contains this key
   - After swapdb when the new db has this key but with different type.
   
In the new implementation the reported errors will be the same as if the command was processed after effect:
**NOGROUP** - in case key no longer exists, or **WRONGTYPE** in case the key was overridden with a different type.

2. Reprocessing the command means that some checks will be reevaluated once the
client is unblocked.
For example, ACL rules might change since the command originally was executed and
will fail once the client is unblocked.
Another example is OOM condition checks which might enable the command to run and
block but fail the command reprocess once the client is unblocked.

3. One of the changes in this PR is that no command stats are being updated once the
command is blocked (all stats will be updated once the client is unblocked). This implies
that when we have many clients blocked, users will no longer be able to get that information
from the command stats. However the information can still be gathered from the client list.

**Client blocking**
---------------------------------------------------

the blocking on key will still be triggered the same way as it is done today.
in order to block the current client on list of keys, the call to
blockForKeys will still need to be made which will perform the same as it is today:

*  add the client to the list of blocked clients on each key
*  keep the key with a matching list node (position in the global blocking clients list for that key)
   in the client private blocking key dict.
*  flag the client with CLIENT_BLOCKED
*  update blocking statistics
*  register the client on the timeout table

**Key Unblock**
---------------------------------------------------

Unblocking a specific key will be triggered (same as today) by calling signalKeyAsReady.
the implementation in that part will stay the same as today - adding the key to the global readyList.
The reason to maintain the readyList (as apposed to iterating over all clients blocked on the specific key)
is in order to keep the signal operation as short as possible, since it is called during the command processing.
The main change is that instead of going through a dedicated code path that operates the blocked command
we will just call processPendingCommandsAndResetClient.

**ClientUnblock (keys)**
---------------------------------------------------

1. Unblocking clients on keys will be triggered after command is
   processed and during the beforeSleep
8. the general schema is:
9. For each key *k* in the readyList:
```            
For each client *c* which is blocked on *k*:
            in case either:
	          1. *k* exists AND the *k* type matches the current client blocking type
	  	      OR
	          2. *k* exists and *c* is blocked on module command
	    	      OR
	          3. *k* does not exists and *c* was blocked with the flag
	             unblock_on_deleted_key
                 do:
                                  1. remove the client from the list of clients blocked on this key
                                  2. remove the blocking list node from the client blocking key dict
                                  3. remove the client from the timeout list
                                  10. queue the client on the unblocked_clients list
                                  11. *NEW*: call processCommandAndResetClient(c);
```
*NOTE:* for module blocked clients we will still call the moduleUnblockClientByHandle
              which will queue the client for processing in moduleUnblockedClients list.

**Process Unblocked clients**
---------------------------------------------------

The process of all unblocked clients is done in the beforeSleep and no change is planned
in that part.

The general schema will be:
For each client *c* in server.unblocked_clients:

        * remove client from the server.unblocked_clients
        * set back the client readHandler
        * continue processing the pending command and input buffer.

*Some notes regarding the new implementation*
---------------------------------------------------

1. Although it was proposed, it is currently difficult to remove the
   read handler from the client while it is blocked.
   The reason is that a blocked client should be unblocked when it is
   disconnected, or we might consume data into void.

2. While this PR mainly keep the current blocking logic as-is, there
   might be some future additions to the infrastructure that we would
   like to have:
   - allow non-preemptive blocking of client - sometimes we can think
     that a new kind of blocking can be expected to not be preempt. for
     example lets imagine we hold some keys on disk and when a command
     needs to process them it will block until the keys are uploaded.
     in this case we will want the client to not disconnect or be
     unblocked until the process is completed (remove the client read
     handler, prevent client timeout, disable unblock via debug command etc...).
   - allow generic blocking based on command declared keys - we might
     want to add a hook before command processing to check if any of the
     declared keys require the command to block. this way it would be
     easier to add new kinds of key-based blocking mechanisms.

Co-authored-by: Oran Agra <oran@redislabs.com>
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
… 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.
@enjoy-binbin
Copy link
Contributor

It looks like we have this problem #12998

when reprocessing the XREADGROUP BLOCK, we will reset the timeout in blockForKeys (since we will get a new timeout when we unblock a key):

void blockForKeys(client *c, int btype, robj **keys, int numkeys, mstime_t timeout, int unblock_on_nokey) {
    dictEntry *db_blocked_entry, *db_blocked_existing_entry, *client_blocked_entry;
    list *l;
    int j;

    c->bstate.timeout = timeout;

In this case, should we first determine whether c->bstate.timeout has a value?

    if (!c->bstate.timeout)
        c->bstate.timeout = timeout;

@oranagra
Copy link
Member

if we do that, we'll need to make sure to reset bstate.timeout in resetClient() and createClient().
but maybe it's a better idea to add a client flag and explicitly let the command know that it is being re-processed?

@enjoy-binbin
Copy link
Contributor

but maybe it's a better idea to add a client flag and explicitly let the command know that it is being re-processed?

That's a good idea, I've thought about it before. But I haven't figured out where to check this flag, something like this?

        c->flags |= CLIENT_RE_PROCESSING_COMMAND;
        processCommandAndResetClient(c);
        c->flags &= ~CLIENT_RE_PROCESSING_COMMAND;

    if (!(c->flags & CLIENT_RE_PROCESSING_COMMAND))
        c->bstate.timeout = timeout;

@oranagra
Copy link
Member

There's a reprocessing flag in call(). Set and clear the client flag there

enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jan 29, 2024
In redis#11012, we will reprocess command when client is unblocked on keys,
In the XREADGROUP BLOCK scenario, because of the re-processing command,
we will recalculate the block timeout, causing the blocking time to be
reset.

This commit add a new CLIENT_REPROCESSING_COMMAND clent flag, explicitly
let the command know that it is being re-processed, later in blockForKeys
we will not reset the timeout.

Fixes redis#12998.
oranagra pushed a commit that referenced this pull request Jan 30, 2024
…13004)

In #11012, we will reprocess command when client is unblocked on keys,
in some blocking commands, for example, in the XREADGROUP BLOCK
scenario,
because of the re-processing command, we will recalculate the block
timeout,
causing the blocking time to be reset.

This commit add a new CLIENT_REPROCESSING_COMMAND clent flag, explicitly
let the command know that it is being re-processed, later in
blockForKeys
we will not reset the timeout.

Affected BLOCK cases: 
- list / zset / stream, added test cases for each.

Unaffected cases:
- module (never re-process the commands).
- WAIT / WAITAOF (never re-process the commands).

Fixes #12998.
roggervalf pushed a commit to roggervalf/redis that referenced this pull request Feb 11, 2024
…edis#13004)

In redis#11012, we will reprocess command when client is unblocked on keys,
in some blocking commands, for example, in the XREADGROUP BLOCK
scenario,
because of the re-processing command, we will recalculate the block
timeout,
causing the blocking time to be reset.

This commit add a new CLIENT_REPROCESSING_COMMAND clent flag, explicitly
let the command know that it is being re-processed, later in
blockForKeys
we will not reset the timeout.

Affected BLOCK cases: 
- list / zset / stream, added test cases for each.

Unaffected cases:
- module (never re-process the commands).
- WAIT / WAITAOF (never re-process the commands).

Fixes redis#12998.
YaacovHazan pushed a commit to YaacovHazan/redis that referenced this pull request May 16, 2024
…edis#13004)

In redis#11012, we will reprocess command when client is unblocked on keys,
in some blocking commands, for example, in the XREADGROUP BLOCK
scenario,
because of the re-processing command, we will recalculate the block
timeout,
causing the blocking time to be reset.

This commit add a new CLIENT_REPROCESSING_COMMAND clent flag, explicitly
let the command know that it is being re-processed, later in
blockForKeys
we will not reset the timeout.

Affected BLOCK cases:
- list / zset / stream, added test cases for each.

Unaffected cases:
- module (never re-process the commands).
- WAIT / WAITAOF (never re-process the commands).

Fixes redis#12998.

(cherry picked from commit 492021d)
YaacovHazan pushed a commit that referenced this pull request May 19, 2024
…13004)

In #11012, we will reprocess command when client is unblocked on keys,
in some blocking commands, for example, in the XREADGROUP BLOCK
scenario,
because of the re-processing command, we will recalculate the block
timeout,
causing the blocking time to be reset.

This commit add a new CLIENT_REPROCESSING_COMMAND clent flag, explicitly
let the command know that it is being re-processed, later in
blockForKeys
we will not reset the timeout.

Affected BLOCK cases:
- list / zset / stream, added test cases for each.

Unaffected cases:
- module (never re-process the commands).
- WAIT / WAITAOF (never re-process the commands).

Fixes #12998.

(cherry picked from commit 492021d)
funny-dog pushed a commit to funny-dog/redis that referenced this pull request Sep 17, 2025
…edis#13004)

In redis#11012, we will reprocess command when client is unblocked on keys,
in some blocking commands, for example, in the XREADGROUP BLOCK
scenario,
because of the re-processing command, we will recalculate the block
timeout,
causing the blocking time to be reset.

This commit add a new CLIENT_REPROCESSING_COMMAND clent flag, explicitly
let the command know that it is being re-processed, later in
blockForKeys
we will not reset the timeout.

Affected BLOCK cases: 
- list / zset / stream, added test cases for each.

Unaffected cases:
- module (never re-process the commands).
- WAIT / WAITAOF (never re-process the commands).

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

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Refactor blocking framework

9 participants