Skip to content

Make sure we do not propagate nested MULTI/EXEC#8097

Merged
oranagra merged 3 commits intoredis:unstablefrom
guybe7:module_double_multi
Dec 6, 2020
Merged

Make sure we do not propagate nested MULTI/EXEC#8097
oranagra merged 3 commits intoredis:unstablefrom
guybe7:module_double_multi

Conversation

@guybe7
Copy link
Collaborator

@guybe7 guybe7 commented Nov 25, 2020

Fix #8049

@guybe7
Copy link
Collaborator Author

guybe7 commented Nov 25, 2020

for the record: a cleaner solution would be if the RM_Call client would inherit the CLIENT_MULTI flag from the "caller" (so we won't need if (server.propagate_in_transaction) return;
the problem is the "caller" is notifyKeyspaceEvent... if we want to go this way we would need notifyKeyspaceEvent to get a client arg and pass it to moduleNotifyKeyspaceEvent

@guybe7
Copy link
Collaborator Author

guybe7 commented Nov 25, 2020

@oranagra i'm using a server var inside moduleReplicateMultiIfNeeded - maybe we should be concerned about threads?

@guybe7 guybe7 requested a review from oranagra November 25, 2020 15:54
@oranagra
Copy link
Member

@guybe7 i suppose what's stopping you from taking the "cleaner solution" is that you didn't want to pass an extra argument to all the calls to notifyKeyspaceEvent. but why can't we use server.current_client?

src/module.c Outdated
* RedisModuleCommandDispatcher() function. */
void moduleReplicateMultiIfNeeded(RedisModuleCtx *ctx) {
/* Skip this if server has already emitted MULTI */
if (server.propagate_in_transaction) return;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe CLIENT_MULTI|CLIENT_LUA is redundant, add assertio

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.

@guybe7 please edit the top comment to reflect all the changes / implications of this PR.

@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Nov 29, 2020
@oranagra oranagra merged commit 1df5bb5 into redis:unstable Dec 6, 2020
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Dec 6, 2020
@oranagra oranagra mentioned this pull request Jan 13, 2021
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Mar 2, 2021
One way this was happening is when a module issued an RM_Call which would inject MULTI.
If the module command that does that was itself issued by something else that already did
added MULTI (e.g. another module, or a Lua script), it would have caused nested MULTI.

In fact the MULTI state in the client or the MULTI_EMITTED flag in the context isn't
the right indication that we need to propagate MULTI or not, because on a nested calls
(possibly a module action called by a keyspace event of another module action), these
flags aren't retained / reflected.

instead there's now a global propagate_in_transaction flag for that.

in addition to that, we now have a global in_eval and in_exec flags, to serve the flags
of RM_GetContextFlags, since their dependence on the current client is wrong for the same
reasons mentioned above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] replica receive nested "multi" command

2 participants