Skip to content

Fix RM_Yield bug processing future commands of the current client.#10573

Merged
oranagra merged 2 commits intoredis:unstablefrom
oranagra:rm_yield_protect_client
Apr 18, 2022
Merged

Fix RM_Yield bug processing future commands of the current client.#10573
oranagra merged 2 commits intoredis:unstablefrom
oranagra:rm_yield_protect_client

Conversation

@oranagra
Copy link
Member

RM_Yield was missing a call to protectClient to prevent redis from
processing future commands of the yielding client.

Adding tests that fail without this fix.

This would be complicated to solve since nested calls to RM_Call used to
replace the current_client variable with the module temp client.

It looks like it's no longer necessary to do that, since it was added
back in #9890 to solve two issues, both already gone:

  1. call to CONFIG SET maxmemory could trigger a module hook calling
    RM_Call. although this specific issue is gone, arguably other hooks
    like keyspace notification, can do the same.
  2. an assertion in lookupKey that checks the current command of the
    current client, introduced in Sort out the mess around writable replicas and lookupKeyRead/Write #9572 and removed in Remove assert and refuse delete expired on ro replicas #10248

RM_Yield was missing a call to protectClient to prevent redis from
processing future commands of the yielding client.

Adding tests that fail without this fix.

This would be complicated to solve since nested calls to RM_Call used to
replace the current_client variable with the module temp client.

It looks like it's no longer necessary to do that, since it was added
back in #9890 to solve two issues, both already gone:
1. call to CONFIG SET maxmemory could trigger a module hook calling
   RM_Call. although this specific issue is gone, arguably other hooks
   like keyspace notification, can do the same.
2. an assertion in lookupKey that checks the current command of the
   current client, introduced in #9572 and removed in #10248
Co-authored-by: zhaozhao.zz <[email protected]>
@soloestoy
Copy link
Contributor

soloestoy commented Apr 13, 2022

If I understand correctly, this PR addresses the nested RM_Call() issue, but doesn't handle background thread in module like call RM_Yield in blocked module client, it's unnecessary or we just consider it as an abuse case?

if (server.busy_module_yield_flags) {
blockingOperationEnds();
server.busy_module_yield_flags = BUSY_MODULE_YIELD_NONE;
if (server.current_client)
Copy link
Contributor

Choose a reason for hiding this comment

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

here server.current_client is always NULL I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, i thought so too, but then was worried maybe someone's using these contexts inside the redis's main thread while executing a command.
@MeirShpilraien @guybe7 please comment.

Choose a reason for hiding this comment

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

I think that unlocking the GIL on the Redis main thread is a miss-use of the API...

Copy link
Member Author

Choose a reason for hiding this comment

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

so shall i remove the protect / unprotect calls from here?
maybe add an assertion for currecnt_client being NULL?

Copy link
Member Author

Choose a reason for hiding this comment

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

i guess i'd rather leave it there even if it might be dead code..

@oranagra oranagra requested review from soloestoy and yossigo April 18, 2022 10:08
@oranagra oranagra merged commit 7d1ad6c into redis:unstable Apr 18, 2022
@oranagra oranagra deleted the rm_yield_protect_client branch April 18, 2022 11:56
oranagra pushed a commit that referenced this pull request Apr 25, 2022
This case is interesting because it originates from cron,
rather than from another command.

The idea came from looking at #9890 and #10573, and I was wondering if RM_Call
would work properly when `server.current_client == NULL`
@oranagra oranagra mentioned this pull request Apr 27, 2022
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
…edis#10573)

RM_Yield was missing a call to protectClient to prevent redis from
processing future commands of the yielding client.

Adding tests that fail without this fix.

This would be complicated to solve since nested calls to RM_Call used to
replace the current_client variable with the module temp client.

It looks like it's no longer necessary to do that, since it was added
back in redis#9890 to solve two issues, both already gone:
1. call to CONFIG SET maxmemory could trigger a module hook calling
   RM_Call. although this specific issue is gone, arguably other hooks
   like keyspace notification, can do the same.
2. an assertion in lookupKey that checks the current command of the
   current client, introduced in redis#9572 and removed in redis#10248
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
This case is interesting because it originates from cron,
rather than from another command.

The idea came from looking at redis#9890 and redis#10573, and I was wondering if RM_Call
would work properly when `server.current_client == NULL`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants