Add a special notification unlink available only for modules #9406

huangzhw · 2021-08-23T05:05:12Z

We add a new module event RedisModule_Event_Key, this event is fired when a key is removed from the keyspace.
The event includes an open key that can be used for reading the key before it is removed.
Modules can also extract the key-name, and use RM_Open or RM_Call to access key from within that event, but shouldn't modify anything from within this event.
The following sub events are available:

REDISMODULE_SUBEVENT_KEY_DELETED
REDISMODULE_SUBEVENT_KEY_EXPIRED
REDISMODULE_SUBEVENT_KEY_EVICTED
REDISMODULE_SUBEVENT_KEY_OVERWRITE

The data pointer can be casted to a RedisModuleKeyInfo structure with the following fields:

     RedisModuleKey *key;    // Opened Key

internals

We also add two dict functions:
dictTwoPhaseUnlinkFind finds an element from the table, also get the plink of the entry.
The entry is returned if the element is found. The user should later call dictTwoPhaseUnlinkFree
with it in order to unlink and release it. Otherwise if the key is not found, NULL is returned.
These two functions should be used in pair. dictTwoPhaseUnlinkFind pauses rehash and
dictTwoPhaseUnlinkFree resumes rehash.
We change dbOverwrite to dbReplaceValue which just replaces the value of the key and
doesn't fire any events. The "overwrite" part (which emits events) is just when called from setKey,
the other places that called dbOverwrite were ones that just update the value in-place (INCR*, SPOP,
and dbUnshareStringValue). This should not have any real impact since moduleNotifyKeyUnlink and
signalDeletedKeyAsReady wouldn't have mattered in these cases anyway (i.e. module keys and
stream keys didn't have direct calls to dbOverwrite)
since we allow doing RM_OpenKey from withing these callbacks, we temporarily disable lazy expiry.
We also temporarily disable lazy expiry when we are in unlink/unlink2 callback and keyspace
notification callback.
Move special definitions to the top of redismodule.h
This is needed to resolve compilation errors with RedisModuleKeyInfoV1
that carries a RedisModuleKey member.

oranagra

only after reviewing, i realized that this completely misses the request of being able to access the value from that notification callback.
for that to work, the callback needs to be made in dbSyncDelete and dbAsyncDelete before the call for dictUnlink.
but on the other hand, we can't afford to do that if the key doesn't exist, so it seems we'll have to add an additional dictFind that's only used if there's some module subscribed to that event.

also, it's probably a good idea to add a test that shows that's working (RM_OpenKey getting the value)

src/redismodule.h

src/module.c

src/server.h

tests/modules/keyspace_events.c

huangzhw · 2021-08-23T11:09:59Z

I think about three ways:

We add dictFind that's only used if there's some module subscribed to that event
unlink2 is used for module types. Whether we can use it for other native types?
Add notify_callback2 take addition value for module.

oranagra · 2021-08-23T12:33:56Z

the last two suggestions are only valid if we're only talking about module values (specifically the module that created that key).
i think the request was about keyspace notification since people may want to use it on other types of keys.

huangzhw · 2021-08-24T00:40:35Z

When this notification triggered by lpop hdel, etc, the object is zero length. I suspect whether this is useful.
If notification callback can access the object to be deleted, it can delete it or do something else. I think it's dangerous.
If do it, I think we can add two function

dictEntry *dictFindWithPlink(dict *d, const void *key, dictEntry ***plink)
// We can't modify db between this
void dictUnlinkWithPlink(dict *d,  dictEntry *entry, dictEntry **plink)

oranagra · 2021-08-24T05:38:29Z

interesting idea. so it's like when we broke dictDelete into two: dictUnlink and dictFreeUnlinkedEntry.
but this time we break it in a different place (before the unlinking).

you're right, that this API is becoming more dangerous the more we think of it:

as you said, the key the module may access may be in an odd state (like empty list)
it can even be in a state that can lead to a crash, maybe o->ptr was already released, but not set to NULL.
obviously the module can't modify the key
the module can't even do any other database modifications since that can get plink out of sync.
even if we only expect the module to do read-only database lookup, we have to disable the incremental dict rehashing in that sensitive time (otherwise dictFind can mess things up).

1, 3 and 4 don't worry me too much, we'll document these and if the module violates that, that's they're mess to handle.
5 is messy but actually easily solvable.
currently 2 worries me the most.

@guybe7 @MeirShpilraien any advise?

guybe7 · 2021-08-24T07:23:40Z

@oranagra can you give an example of (2)?

oranagra · 2021-08-24T07:43:52Z

no, i can't find any.. maybe i'm wrong and it doesn't exist..
it'll eventually do decrRefcount, which attempts to release o->ptr again, so for sure what i said about freeing it and not setting it to NULL doesn't exist, and i can't currently find anyone that frees and sets it to NULL either.

btw, another interesting case is the RENAME command. it does call dbDelete, but it adds the same object (and increments the refcount) before the dbDelete call.

i suppose that in this case there's a logical deletion and addition, but we're not gonna send the removed notification in that case, and we assume the module will explicitly handle that.

guybe7 · 2021-08-24T09:20:50Z

@oranagra why aren't we gonna send the remove notification in that case? even though we don't actually release the memory, moduleNotifyKeyUnlink is still called (both for the src and the dst, if exists)
or maybe I'm missing something?

oranagra · 2021-08-24T09:33:34Z

@guybe7 i don't see any call to moduleNotifyKeyUnlink in RENAME

guybe7 · 2021-08-24T09:39:00Z

@oranagra it's inside dbDeleteSync called by dbDelete (which is called up to two times in renameCommand)

oranagra · 2021-08-24T09:54:16Z

@guybe7 the first call is the overwrite, so that's a plain deletion and not part of this discussion.
the second deletion will indeed send that notification (don't recall what was going though my head earlier).

so anyway, i guess we can consider that second notification a problem, and may wanna hide it.
on one hand, logically we're deleting one key and adding another, so the removed notification is due.
but on the other hand, that same object is already referenced in the dictionary elsewhere.

so either we make sure to disable the notification in that case and let the module handle RENAME notification differently,
or maybe we wanna incrRefcount of the robj, then do the deletion, and only last add it to the new key.
whatever we do, we may need to handle MOVE too.

huangzhw · 2021-08-28T13:29:27Z

The function name maybe should change.
I still wonder whether this is OK. It is a little ugly and potential dangerous.

src/dict.c

src/lazyfree.c

src/module.c

tests/modules/keyspace_events.c

src/dict.c

the key was unlink from keyspace before real delete.

src/lazyfree.c

src/db.c

dbUnshareStringValue doesn't intend to replace the existing key with a new one. So it should not call any module hooks, keyspace notifications, and so on.

src/module.c

tests/modules/hooks.c

add RM_CALL RM_OpenKey tests

oranagra · 2022-11-30T09:58:37Z

@huangzhw merged (more than a year after the initial version).
thank you for the patients and for following all the requests.

…11199) ### Summary of API additions * `RedisModule_AddPostNotificationJob` - new API to call inside a key space notification (and on more locations in the future) and allow to add a post job as describe above. * New module option, `REDISMODULE_OPTIONS_ALLOW_NESTED_KEYSPACE_NOTIFICATIONS`, allows to disable Redis protection of nested key-space notifications. * `RedisModule_GetModuleOptionsAll` - gets the mask of all supported module options so a module will be able to check if a given option is supported by the current running Redis instance. ### Background The following PR is a proposal of handling write operations inside module key space notifications. After a lot of discussions we came to a conclusion that module should not perform any write operations on key space notification. Some examples of issues that such write operation can cause are describe on the following links: * Bad replication oreder - redis#10969 * Used after free - redis#10969 (comment) * Used after free - redis#9406 (comment) There are probably more issues that are yet to be discovered. The underline problem with writing inside key space notification is that the notification runs synchronously, this means that the notification code will be executed in the middle on Redis logic (commands logic, eviction, expire). Redis **do not assume** that the data might change while running the logic and such changes can crash Redis or cause unexpected behaviour. The solution is to state that modules **should not** perform any write command inside key space notification (we can chose whether or not we want to force it). To still cover the use-case where module wants to perform a write operation as a reaction to key space notifications, we introduce a new API , `RedisModule_AddPostNotificationJob`, that allows to register a callback that will be called by Redis when the following conditions hold: * It is safe to perform any write operation. * The job will be called atomically along side the operation that triggers it (in our case, key space notification). Module can use this new API to safely perform any write operation and still achieve atomicity between the notification and the write. Although currently the API is supported on key space notifications, the API is written in a generic way so that in the future we will be able to use it on other places (server events for example). ### Technical Details Whenever a module uses `RedisModule_AddPostNotificationJob` the callback is added to a list of callbacks (called `modulePostExecUnitJobs`) that need to be invoke after the current execution unit ends (whether its a command, eviction, or active expire). In order to trigger those callback atomically with the notification effect, we call those callbacks on `postExecutionUnitOperations` (which was `propagatePendingCommands` before this PR). The new function fires the post jobs and then calls `propagatePendingCommands`. If the callback perform more operations that triggers more key space notifications. Those keys space notifications might register more callbacks. Those callbacks will be added to the end of `modulePostExecUnitJobs` list and will be invoke atomically after the current callback ends. This raises a concerns of entering an infinite loops, we consider infinite loops as a logical bug that need to be fixed in the module, an attempt to protect against infinite loops by halting the execution could result in violation of the feature correctness and so **Redis will make no attempt to protect the module from infinite loops** In addition, currently key space notifications are not nested. Some modules might want to allow nesting key-space notifications. To allow that and keep backward compatibility, we introduce a new module option called `REDISMODULE_OPTIONS_ALLOW_NESTED_KEYSPACE_NOTIFICATIONS`. Setting this option will disable the Redis key-space notifications nesting protection and will pass this responsibility to the module. ### Redis infrastructure This PR promotes the existing `propagatePendingCommands` to an "Execution Unit" concept, which is called after each atomic unit of execution, Co-authored-by: Oran Agra <oran@redislabs.com> Co-authored-by: Yossi Gottlieb <yossigo@gmail.com> Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>

) Add a new module event `RedisModule_Event_Key`, this event is fired when a key is removed from the keyspace. The event includes an open key that can be used for reading the key before it is removed. Modules can also extract the key-name, and use RM_Open or RM_Call to access key from within that event, but shouldn't modify anything from within this event. The following sub events are available: - `REDISMODULE_SUBEVENT_KEY_DELETED` - `REDISMODULE_SUBEVENT_KEY_EXPIRED` - `REDISMODULE_SUBEVENT_KEY_EVICTED` - `REDISMODULE_SUBEVENT_KEY_OVERWRITE` The data pointer can be casted to a RedisModuleKeyInfo structure with the following fields: ``` RedisModuleKey *key; // Opened Key ``` ### internals * We also add two dict functions: `dictTwoPhaseUnlinkFind` finds an element from the table, also get the plink of the entry. The entry is returned if the element is found. The user should later call `dictTwoPhaseUnlinkFree` with it in order to unlink and release it. Otherwise if the key is not found, NULL is returned. These two functions should be used in pair. `dictTwoPhaseUnlinkFind` pauses rehash and `dictTwoPhaseUnlinkFree` resumes rehash. * We change `dbOverwrite` to `dbReplaceValue` which just replaces the value of the key and doesn't fire any events. The "overwrite" part (which emits events) is just when called from `setKey`, the other places that called dbOverwrite were ones that just update the value in-place (INCR*, SPOP, and dbUnshareStringValue). This should not have any real impact since `moduleNotifyKeyUnlink` and `signalDeletedKeyAsReady` wouldn't have mattered in these cases anyway (i.e. module keys and stream keys didn't have direct calls to dbOverwrite) * since we allow doing RM_OpenKey from withing these callbacks, we temporarily disable lazy expiry. * We also temporarily disable lazy expiry when we are in unlink/unlink2 callback and keyspace notification callback. * Move special definitions to the top of redismodule.h This is needed to resolve compilation errors with RedisModuleKeyInfoV1 that carries a RedisModuleKey member. Co-authored-by: Oran Agra <oran@redislabs.com>

…11199) ### Summary of API additions * `RedisModule_AddPostNotificationJob` - new API to call inside a key space notification (and on more locations in the future) and allow to add a post job as describe above. * New module option, `REDISMODULE_OPTIONS_ALLOW_NESTED_KEYSPACE_NOTIFICATIONS`, allows to disable Redis protection of nested key-space notifications. * `RedisModule_GetModuleOptionsAll` - gets the mask of all supported module options so a module will be able to check if a given option is supported by the current running Redis instance. ### Background The following PR is a proposal of handling write operations inside module key space notifications. After a lot of discussions we came to a conclusion that module should not perform any write operations on key space notification. Some examples of issues that such write operation can cause are describe on the following links: * Bad replication oreder - redis#10969 * Used after free - redis#10969 (comment) * Used after free - redis#9406 (comment) There are probably more issues that are yet to be discovered. The underline problem with writing inside key space notification is that the notification runs synchronously, this means that the notification code will be executed in the middle on Redis logic (commands logic, eviction, expire). Redis **do not assume** that the data might change while running the logic and such changes can crash Redis or cause unexpected behaviour. The solution is to state that modules **should not** perform any write command inside key space notification (we can chose whether or not we want to force it). To still cover the use-case where module wants to perform a write operation as a reaction to key space notifications, we introduce a new API , `RedisModule_AddPostNotificationJob`, that allows to register a callback that will be called by Redis when the following conditions hold: * It is safe to perform any write operation. * The job will be called atomically along side the operation that triggers it (in our case, key space notification). Module can use this new API to safely perform any write operation and still achieve atomicity between the notification and the write. Although currently the API is supported on key space notifications, the API is written in a generic way so that in the future we will be able to use it on other places (server events for example). ### Technical Details Whenever a module uses `RedisModule_AddPostNotificationJob` the callback is added to a list of callbacks (called `modulePostExecUnitJobs`) that need to be invoke after the current execution unit ends (whether its a command, eviction, or active expire). In order to trigger those callback atomically with the notification effect, we call those callbacks on `postExecutionUnitOperations` (which was `propagatePendingCommands` before this PR). The new function fires the post jobs and then calls `propagatePendingCommands`. If the callback perform more operations that triggers more key space notifications. Those keys space notifications might register more callbacks. Those callbacks will be added to the end of `modulePostExecUnitJobs` list and will be invoke atomically after the current callback ends. This raises a concerns of entering an infinite loops, we consider infinite loops as a logical bug that need to be fixed in the module, an attempt to protect against infinite loops by halting the execution could result in violation of the feature correctness and so **Redis will make no attempt to protect the module from infinite loops** In addition, currently key space notifications are not nested. Some modules might want to allow nesting key-space notifications. To allow that and keep backward compatibility, we introduce a new module option called `REDISMODULE_OPTIONS_ALLOW_NESTED_KEYSPACE_NOTIFICATIONS`. Setting this option will disable the Redis key-space notifications nesting protection and will pass this responsibility to the module. ### Redis infrastructure This PR promotes the existing `propagatePendingCommands` to an "Execution Unit" concept, which is called after each atomic unit of execution, Co-authored-by: Oran Agra <oran@redislabs.com> Co-authored-by: Yossi Gottlieb <yossigo@gmail.com> Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>

) Add a new module event `RedisModule_Event_Key`, this event is fired when a key is removed from the keyspace. The event includes an open key that can be used for reading the key before it is removed. Modules can also extract the key-name, and use RM_Open or RM_Call to access key from within that event, but shouldn't modify anything from within this event. The following sub events are available: - `REDISMODULE_SUBEVENT_KEY_DELETED` - `REDISMODULE_SUBEVENT_KEY_EXPIRED` - `REDISMODULE_SUBEVENT_KEY_EVICTED` - `REDISMODULE_SUBEVENT_KEY_OVERWRITE` The data pointer can be casted to a RedisModuleKeyInfo structure with the following fields: ``` RedisModuleKey *key; // Opened Key ``` ### internals * We also add two dict functions: `dictTwoPhaseUnlinkFind` finds an element from the table, also get the plink of the entry. The entry is returned if the element is found. The user should later call `dictTwoPhaseUnlinkFree` with it in order to unlink and release it. Otherwise if the key is not found, NULL is returned. These two functions should be used in pair. `dictTwoPhaseUnlinkFind` pauses rehash and `dictTwoPhaseUnlinkFree` resumes rehash. * We change `dbOverwrite` to `dbReplaceValue` which just replaces the value of the key and doesn't fire any events. The "overwrite" part (which emits events) is just when called from `setKey`, the other places that called dbOverwrite were ones that just update the value in-place (INCR*, SPOP, and dbUnshareStringValue). This should not have any real impact since `moduleNotifyKeyUnlink` and `signalDeletedKeyAsReady` wouldn't have mattered in these cases anyway (i.e. module keys and stream keys didn't have direct calls to dbOverwrite) * since we allow doing RM_OpenKey from withing these callbacks, we temporarily disable lazy expiry. * We also temporarily disable lazy expiry when we are in unlink/unlink2 callback and keyspace notification callback. * Move special definitions to the top of redismodule.h This is needed to resolve compilation errors with RedisModuleKeyInfoV1 that carries a RedisModuleKey member. Co-authored-by: Oran Agra <oran@redislabs.com>

oranagra · 2023-05-08T13:57:53Z

src/module.c

+    } else if (flags & DB_FLAG_KEY_OVERWRITE) {
+        subevent = REDISMODULE_SUBEVENT_KEY_OVERWRITTEN;
+    }
+    KeyInfo info = {dbid, key, val, REDISMODULE_WRITE};


anyone understands / remembers why we used REDISMODULE_WRITE here?

seems like it should be READ since the module should not try to modify the key in the CB of this event

…#12156) just a plain overlook by #9406

We should emit DB_FLAG_KEY_EXPIRED instead of DB_FLAG_KEY_DELETED. This is an overlook in redis#9406.

…12243) We should emit DB_FLAG_KEY_EXPIRED instead of DB_FLAG_KEY_DELETED. This is an overlook in #9406.

…11199) ### Summary of API additions * `RedisModule_AddPostNotificationJob` - new API to call inside a key space notification (and on more locations in the future) and allow to add a post job as describe above. * New module option, `REDISMODULE_OPTIONS_ALLOW_NESTED_KEYSPACE_NOTIFICATIONS`, allows to disable Redis protection of nested key-space notifications. * `RedisModule_GetModuleOptionsAll` - gets the mask of all supported module options so a module will be able to check if a given option is supported by the current running Redis instance. ### Background The following PR is a proposal of handling write operations inside module key space notifications. After a lot of discussions we came to a conclusion that module should not perform any write operations on key space notification. Some examples of issues that such write operation can cause are describe on the following links: * Bad replication oreder - redis#10969 * Used after free - redis#10969 (comment) * Used after free - redis#9406 (comment) There are probably more issues that are yet to be discovered. The underline problem with writing inside key space notification is that the notification runs synchronously, this means that the notification code will be executed in the middle on Redis logic (commands logic, eviction, expire). Redis **do not assume** that the data might change while running the logic and such changes can crash Redis or cause unexpected behaviour. The solution is to state that modules **should not** perform any write command inside key space notification (we can chose whether or not we want to force it). To still cover the use-case where module wants to perform a write operation as a reaction to key space notifications, we introduce a new API , `RedisModule_AddPostNotificationJob`, that allows to register a callback that will be called by Redis when the following conditions hold: * It is safe to perform any write operation. * The job will be called atomically along side the operation that triggers it (in our case, key space notification). Module can use this new API to safely perform any write operation and still achieve atomicity between the notification and the write. Although currently the API is supported on key space notifications, the API is written in a generic way so that in the future we will be able to use it on other places (server events for example). ### Technical Details Whenever a module uses `RedisModule_AddPostNotificationJob` the callback is added to a list of callbacks (called `modulePostExecUnitJobs`) that need to be invoke after the current execution unit ends (whether its a command, eviction, or active expire). In order to trigger those callback atomically with the notification effect, we call those callbacks on `postExecutionUnitOperations` (which was `propagatePendingCommands` before this PR). The new function fires the post jobs and then calls `propagatePendingCommands`. If the callback perform more operations that triggers more key space notifications. Those keys space notifications might register more callbacks. Those callbacks will be added to the end of `modulePostExecUnitJobs` list and will be invoke atomically after the current callback ends. This raises a concerns of entering an infinite loops, we consider infinite loops as a logical bug that need to be fixed in the module, an attempt to protect against infinite loops by halting the execution could result in violation of the feature correctness and so **Redis will make no attempt to protect the module from infinite loops** In addition, currently key space notifications are not nested. Some modules might want to allow nesting key-space notifications. To allow that and keep backward compatibility, we introduce a new module option called `REDISMODULE_OPTIONS_ALLOW_NESTED_KEYSPACE_NOTIFICATIONS`. Setting this option will disable the Redis key-space notifications nesting protection and will pass this responsibility to the module. ### Redis infrastructure This PR promotes the existing `propagatePendingCommands` to an "Execution Unit" concept, which is called after each atomic unit of execution, Co-authored-by: Oran Agra <oran@redislabs.com> Co-authored-by: Yossi Gottlieb <yossigo@gmail.com> Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>

) Add a new module event `RedisModule_Event_Key`, this event is fired when a key is removed from the keyspace. The event includes an open key that can be used for reading the key before it is removed. Modules can also extract the key-name, and use RM_Open or RM_Call to access key from within that event, but shouldn't modify anything from within this event. The following sub events are available: - `REDISMODULE_SUBEVENT_KEY_DELETED` - `REDISMODULE_SUBEVENT_KEY_EXPIRED` - `REDISMODULE_SUBEVENT_KEY_EVICTED` - `REDISMODULE_SUBEVENT_KEY_OVERWRITE` The data pointer can be casted to a RedisModuleKeyInfo structure with the following fields: ``` RedisModuleKey *key; // Opened Key ``` ### internals * We also add two dict functions: `dictTwoPhaseUnlinkFind` finds an element from the table, also get the plink of the entry. The entry is returned if the element is found. The user should later call `dictTwoPhaseUnlinkFree` with it in order to unlink and release it. Otherwise if the key is not found, NULL is returned. These two functions should be used in pair. `dictTwoPhaseUnlinkFind` pauses rehash and `dictTwoPhaseUnlinkFree` resumes rehash. * We change `dbOverwrite` to `dbReplaceValue` which just replaces the value of the key and doesn't fire any events. The "overwrite" part (which emits events) is just when called from `setKey`, the other places that called dbOverwrite were ones that just update the value in-place (INCR*, SPOP, and dbUnshareStringValue). This should not have any real impact since `moduleNotifyKeyUnlink` and `signalDeletedKeyAsReady` wouldn't have mattered in these cases anyway (i.e. module keys and stream keys didn't have direct calls to dbOverwrite) * since we allow doing RM_OpenKey from withing these callbacks, we temporarily disable lazy expiry. * We also temporarily disable lazy expiry when we are in unlink/unlink2 callback and keyspace notification callback. * Move special definitions to the top of redismodule.h This is needed to resolve compilation errors with RedisModuleKeyInfoV1 that carries a RedisModuleKey member. Co-authored-by: Oran Agra <oran@redislabs.com>

…de an execution unit. This [PR](redis#9406) introduces new server event, `RedisModuleEvent_Key`. This new event allows to read a key data just before it removed from the database (either deleted, expired, evicted, or overwritten). When the key is removed from the database, either by active expire or eviction. The new event was not called as part of an execution unit. This can cause an issue if the module registers a post notification job inside the event. This job will not be executed atomically with the expiration/eviction operation and will not replicated inside a Multi/Exec. Moreover, the post notification job will be executed right after the event where it is still not safe to perform any write operation, this will violate the promise that post notification job will be called atomically with the operation that triggered it and **only when it is safe to write**. The PR fixes the issue by wrapping each expiration/eviction of a key with an execution unit. This make sure the entire operation will run atomically and all the post notification jobs will be executed at the end where it safe to write. Tests were modified to verify the fix.

…de an execution unit. (#12733) Redis 7.2 (#9406) introduced a new modules event, `RedisModuleEvent_Key`. This new event allows the module to read the key data just before it is removed from the database (either deleted, expired, evicted, or overwritten). When the key is removed from the database, either by active expire or eviction. The new event was not called as part of an execution unit. This can cause an issue if the module registers a post notification job inside the event. This job will not be executed atomically with the expiration/eviction operation and will not replicated inside a Multi/Exec. Moreover, the post notification job will be executed right after the event where it is still not safe to perform any write operation, this will violate the promise that post notification job will be called atomically with the operation that triggered it and **only when it is safe to write**. This PR fixes the issue by wrapping each expiration/eviction of a key with an execution unit. This makes sure the entire operation will run atomically and all the post notification jobs will be executed at the end where it is safe to write. Tests were modified to verify the fix.

…de an execution unit. (redis#12733) Redis 7.2 (redis#9406) introduced a new modules event, `RedisModuleEvent_Key`. This new event allows the module to read the key data just before it is removed from the database (either deleted, expired, evicted, or overwritten). When the key is removed from the database, either by active expire or eviction. The new event was not called as part of an execution unit. This can cause an issue if the module registers a post notification job inside the event. This job will not be executed atomically with the expiration/eviction operation and will not replicated inside a Multi/Exec. Moreover, the post notification job will be executed right after the event where it is still not safe to perform any write operation, this will violate the promise that post notification job will be called atomically with the operation that triggered it and **only when it is safe to write**. This PR fixes the issue by wrapping each expiration/eviction of a key with an execution unit. This makes sure the entire operation will run atomically and all the post notification jobs will be executed at the end where it is safe to write. Tests were modified to verify the fix. (cherry picked from commit 0ffb9d2)

…de an execution unit. (#12733) Redis 7.2 (#9406) introduced a new modules event, `RedisModuleEvent_Key`. This new event allows the module to read the key data just before it is removed from the database (either deleted, expired, evicted, or overwritten). When the key is removed from the database, either by active expire or eviction. The new event was not called as part of an execution unit. This can cause an issue if the module registers a post notification job inside the event. This job will not be executed atomically with the expiration/eviction operation and will not replicated inside a Multi/Exec. Moreover, the post notification job will be executed right after the event where it is still not safe to perform any write operation, this will violate the promise that post notification job will be called atomically with the operation that triggered it and **only when it is safe to write**. This PR fixes the issue by wrapping each expiration/eviction of a key with an execution unit. This makes sure the entire operation will run atomically and all the post notification jobs will be executed at the end where it is safe to write. Tests were modified to verify the fix. (cherry picked from commit 0ffb9d2)

oranagra reviewed Aug 23, 2021

View reviewed changes

src/redismodule.h Outdated Show resolved Hide resolved

src/module.c Outdated Show resolved Hide resolved

src/module.c Outdated Show resolved Hide resolved

src/server.h Outdated Show resolved Hide resolved

tests/modules/keyspace_events.c Outdated Show resolved Hide resolved

oranagra added this to Backlog in 7.0 via automation Aug 23, 2021

oranagra moved this from Backlog to In progress in 7.0 Aug 23, 2021

huangzhw changed the title ~~Add a specail notification unlink available only for modules~~ Add a special notification unlink available only for modules Aug 23, 2021

huangzhw marked this pull request as draft August 27, 2021 10:46

oranagra reviewed Aug 30, 2021

View reviewed changes

oranagra moved this from In progress to In Review in 7.0 Aug 30, 2021

oranagra linked an issue Aug 30, 2021 that may be closed by this pull request

access to old value during notification of expiry #9354

Closed

guybe7 reviewed Aug 30, 2021

View reviewed changes

src/dict.c Outdated Show resolved Hide resolved

huangzhw and others added 4 commits September 3, 2021 08:21

Add a specail notification available only for modules, indicates that

884d8c3

the key was unlink from keyspace before real delete.

remove unlink to removed

b26abc3

Don't unlink key when module removed notify

dc4694e

attempt to simplify the plink ref

9226d4e

huangzhw force-pushed the module-notify branch from 574501e to 9226d4e Compare September 6, 2021 00:23

fix comments, delete expire

747cc43

oranagra reviewed Sep 6, 2021

View reviewed changes

src/lazyfree.c Outdated Show resolved Hide resolved

src/db.c Outdated Show resolved Hide resolved

huangzhw mentioned this pull request Sep 12, 2021

dbUnshareStringValue should not call dbOverwrite. #9490

Closed

oranagra mentioned this pull request Sep 13, 2021

access to old value during notification of expiry #9354

Closed

dbUnshareStringValue should not call dbOverwrite.

d4fa9a4

dbUnshareStringValue doesn't intend to replace the existing key with a new one. So it should not call any module hooks, keyspace notifications, and so on.

huangzhw added 4 commits November 29, 2022 19:59

use RedisModuleKey instead of RedisModuleString

ea0e40c

fix

43e1e1e

Merge remote-tracking branch 'official/unstable' into module-notify

9c01b1f

fix

30869eb

oranagra reviewed Nov 29, 2022

View reviewed changes

src/module.c Outdated Show resolved Hide resolved

src/module.c Outdated Show resolved Hide resolved

tests/modules/hooks.c Outdated Show resolved Hide resolved

huangzhw and others added 4 commits November 29, 2022 22:24

fix access local variables

0d854b3

move version

abc06fc

add RM_CALL RM_OpenKey tests

add comments to test

cf8816e

typo

b2b3c04

oranagra merged commit c818131 into redis:unstable Nov 30, 2022

oranagra mentioned this pull request Dec 7, 2022

Explicit propagation of commands #11356

Open

oranagra reviewed May 8, 2023

View reviewed changes

oranagra mentioned this pull request May 11, 2023

fix moduleNotifyKeyUnlink to open the key with READ rather than WRITE #12156

Merged

oranagra added a commit that referenced this pull request May 11, 2023

fix moduleNotifyKeyUnlink to open the key with READ rather than WRITE (…

38b8440

…#12156) just a plain overlook by #9406

enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request May 29, 2023

Fix GETEX db delete call to emit DB_FLAG_KEY_EXPIRED on expiration

e693779

We should emit DB_FLAG_KEY_EXPIRED instead of DB_FLAG_KEY_DELETED. This is an overlook in redis#9406.

enjoy-binbin mentioned this pull request May 29, 2023

Fix GETEX db delete call to emit DB_FLAG_KEY_EXPIRED on expiration #12243

Merged

oranagra pushed a commit that referenced this pull request May 29, 2023

Fix GETEX db delete call to emit DB_FLAG_KEY_EXPIRED on expiration (#…

0dfb025

…12243) We should emit DB_FLAG_KEY_EXPIRED instead of DB_FLAG_KEY_DELETED. This is an overlook in #9406.

guybe7 mentioned this pull request Jun 21, 2023

Module events for key modifications #12335

Open

MeirShpilraien mentioned this pull request Nov 6, 2023

Before evicted and before expired server events are not executed inside an execution unit. #12733

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a special notification unlink available only for modules #9406

Add a special notification unlink available only for modules #9406

huangzhw commented Aug 23, 2021 •

edited by oranagra

oranagra left a comment

huangzhw commented Aug 23, 2021

oranagra commented Aug 23, 2021

huangzhw commented Aug 24, 2021

oranagra commented Aug 24, 2021

guybe7 commented Aug 24, 2021

oranagra commented Aug 24, 2021

guybe7 commented Aug 24, 2021

oranagra commented Aug 24, 2021

guybe7 commented Aug 24, 2021

oranagra commented Aug 24, 2021

huangzhw commented Aug 28, 2021

oranagra commented Nov 30, 2022

oranagra May 8, 2023

guybe7 May 8, 2023

Add a special notification unlink available only for modules #9406

Add a special notification unlink available only for modules #9406

Conversation

huangzhw commented Aug 23, 2021 • edited by oranagra

internals

oranagra left a comment

Choose a reason for hiding this comment

huangzhw commented Aug 23, 2021

oranagra commented Aug 23, 2021

huangzhw commented Aug 24, 2021

oranagra commented Aug 24, 2021

guybe7 commented Aug 24, 2021

oranagra commented Aug 24, 2021

guybe7 commented Aug 24, 2021

oranagra commented Aug 24, 2021

guybe7 commented Aug 24, 2021

oranagra commented Aug 24, 2021

huangzhw commented Aug 28, 2021

oranagra commented Nov 30, 2022

oranagra May 8, 2023

Choose a reason for hiding this comment

guybe7 May 8, 2023

Choose a reason for hiding this comment

huangzhw commented Aug 23, 2021 •

edited by oranagra