Blocked module clients should be aware when a key is deleted #11310

guybe7 · 2022-09-22T17:30:55Z

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:

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)
Make sure afterCommand is called in serveClientsBlockedOnKeyByModule, in order to propagate actions from moduleTryServeClientBlockedOnKey.
handleClientsBlockedOnKeys: call propagatePendingCommands directly after lookupKeyReadWithFlags to prevent a possible lazy-expire DEL from being mixed with any command propagated by the preceding functions.
blockForKeys: Caller can specifiy that it wants to be awakened if key is deleted.
Minor optimizations (use dictAddRaw).
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:

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.
Use the new signalDeletedKeyAsReady where needed

blockedonkey.c + tcl:

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

The use case is a module that wants to implement a blocking command on a key that necessarily exists, and want to unblock the client in case the key is deleted (much like what we implemnted for XREADGROUP in redis#10306) blocked.c: 1. Both module and stream functions are called whether the key exists or not, and 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 in order to prevent a possible lazy-expire DEL to be mized with any command propagated by the preceding functions. db.c: 1. scanDatabaseForDeletedStreams is now scanDatabaseForDeletedKeys and will signalKeyAsReady for any key the was removed from the database or changed type. It is the responsibilty of the code in blocked.c to ignore or to act on deleted/type-changed keys. module.c, redismodule.h: 1. new API RM_SignalKeyAsReadyByDbId to be used when RedisModuleCtx is not provided. blockedonkey.c + tcl: 1. Added test of new capabilites (FSL.BPOPGT now requires the key to exist in order to work)

oranagra

mostly LGTM.
other than paranoia about hidden bugs and edge cases.
let's get more feedback.
@MeirShpilraien @yossigo @soloestoy

src/module.c

ranshid · 2022-10-12T05:42:41Z

@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 :) )

blocked.c: 1. blockForKeys: Caller can specifiy that it wants to be awakened if key is deleted. Minor optimizations (use dictAddRaw). 2. 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. Use the new signalDeletedKeyAsReady where needed module.c, redismodule.h: 1. Add new API RedisModule_BlockClientOnKeysWithFlags so that module can mimic the XREADGROUP behvaiour

src/blocked.c

guybe7 · 2022-10-12T12:59:22Z

@ranshid you are right, please review again (i implemented the mechanism discussed in #11012 (comment))

oranagra

mostly LGTM

src/blocked.c

src/module.c

oranagra · 2022-10-16T06:15:00Z

@redis/core-team please approve

itamarhaber

API looks valid, not a CR

madolson

Optional comments. API LGTM. Did not do a full review.

src/module.c

oranagra · 2022-10-18T16:48:26Z

approved in a core-team meeting

1. On PR redis#11310 there was a change in the way clients blocked on stream/module keys might react in case of key no longer exist. However the logic also introduced an optimization to avoid trigger deketed keys in case no client was blocked with key delet condition. This might be a problem in case several clients are blocked on the key but with different types. In order to fix that I have added a counter to the blocking_keys_unblock_on_nokey so that the entry will be removed also in case all the clients who would like to be triggered on nokey were unblocked. 2. Added statistics to the info "Clients" section to report the: number of blocking keys number of blocking keys which have at least 1 client which would like to be unblocked on nokey. 3. added test for the scenarion described in '1'

…1310) 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 redis#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)

oranagra mentioned this pull request Sep 25, 2022

Module API to allow writes after key space notification hooks #11199

Merged

7 tasks

oranagra approved these changes Sep 25, 2022

View reviewed changes

guybe7 commented Sep 26, 2022

View reviewed changes

src/module.c Outdated Show resolved Hide resolved

guybe7 added 2 commits September 26, 2022 08:52

fix bug

4fab2bd

fix

4468c05

oranagra mentioned this pull request Oct 2, 2022

reprocess command when client is unblocked on keys #11012

Merged

guybe7 added 2 commits October 12, 2022 14:43

Merge remote-tracking branch 'origin/unstable' into xreadgroup_block

125aacb

guybe7 commented Oct 12, 2022

View reviewed changes

src/blocked.c Outdated Show resolved Hide resolved

oranagra reviewed Oct 12, 2022

View reviewed changes

src/blocked.c Outdated Show resolved Hide resolved

src/blocked.c Outdated Show resolved Hide resolved

src/blocked.c Outdated Show resolved Hide resolved

src/blocked.c Outdated Show resolved Hide resolved

src/module.c Outdated Show resolved Hide resolved

CR fixes

e596716

oranagra added release-notes indication that this issue needs to be mentioned in the release notes approval-needed Waiting for core team approval to be merged state:major-decision Requires core team consensus labels Oct 16, 2022

itamarhaber approved these changes Oct 17, 2022

View reviewed changes

madolson approved these changes Oct 17, 2022

View reviewed changes

src/module.c Outdated Show resolved Hide resolved

src/module.c Outdated Show resolved Hide resolved

CR fixes

a1725fe

oranagra merged commit b57fd01 into redis:unstable Oct 18, 2022

Blocked module clients should be aware when a key is deleted #11310

Blocked module clients should be aware when a key is deleted #11310

Uh oh!

Conversation

guybe7 commented Sep 22, 2022 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Detailed description of code changes

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ranshid commented Oct 12, 2022

Uh oh!

Uh oh!

guybe7 commented Oct 12, 2022

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Oct 16, 2022

Uh oh!

itamarhaber left a comment

Choose a reason for hiding this comment

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oranagra commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

guybe7 commented Sep 22, 2022 •

edited by oranagra

Loading