redis icon indicating copy to clipboard operation
redis copied to clipboard

Blocked module clients should be aware when a key is deleted

Open guybe7 opened this issue 3 years ago • 3 comments

The use case is a module that wants to implement a blocking command on a key that necessarily exists and wants to unblock the client in case the key is deleted (much like what we implemented for XREADGROUP in #10306)

New module API:

  • RedisModule_BlockClientOnKeysWithFlags

Flags:

  • REDISMODULE_BLOCK_UNBLOCK_NONE
  • REDISMODULE_BLOCK_UNBLOCK_DELETED

Detailed description of code changes

blocked.c:

  1. Both module and stream functions are called whether the key exists or not, regardless of its type. We do that in order to allow modules/stream to unblock the client in case the key is no longer present or has changed type (the behavior for streams didn't change, just code that moved into serveClientsBlockedOnStreamKey)
  2. Make sure afterCommand is called in serveClientsBlockedOnKeyByModule, in order to propagate actions from moduleTryServeClientBlockedOnKey.
  3. handleClientsBlockedOnKeys: call propagatePendingCommands directly after lookupKeyReadWithFlags to prevent a possible lazy-expire DEL from being mixed with any command propagated by the preceding functions.
  4. blockForKeys: Caller can specifiy that it wants to be awakened if key is deleted. Minor optimizations (use dictAddRaw).
  5. signalKeyAsReady became signalKeyAsReadyLogic which can take a boolean in case the key is deleted. It will only signal if there's at least one client that awaits key deletion (to save calls to handleClientsBlockedOnKeys). Minor optimizations (use dictAddRaw)

db.c:

  1. scanDatabaseForDeletedStreams is now scanDatabaseForDeletedKeys and will signalKeyAsReady for any key that was removed from the database or changed type. It is the responsibility of the code in blocked.c to ignore or act on deleted/type-changed keys.
  2. Use the new signalDeletedKeyAsReady where needed

blockedonkey.c + tcl:

  1. Added test of new capabilities (FSL.BPOPGT now requires the key to exist in order to work)

guybe7 avatar Sep 22 '22 17:09 guybe7

@guybe7 I went shortly through the change (will make a deeper attempt later) but do we also plan to change the dbOverwrite? Currently it will only trigger stream types. I read the top comment:

Both module and stream functions are called whether the key exists or not, regardless of its type. We do that in order to allow modules/stream to unblock the client in case the key is no longer present or has changed type (the behavior for streams didn't change, just code that moved into serveClientsBlockedOnStreamKey)

So basically in case of type change due to swapdb the module will be unblocked, but in case of overwrite it will not? (Or I missed something :) )

ranshid avatar Oct 12 '22 05:10 ranshid

@ranshid you are right, please review again (i implemented the mechanism discussed in https://github.com/redis/redis/pull/11012#discussion_r987664102)

guybe7 avatar Oct 12 '22 12:10 guybe7

@redis/core-team please approve

oranagra avatar Oct 16 '22 06:10 oranagra

approved in a core-team meeting

oranagra avatar Oct 18 '22 16:10 oranagra