Fix RM_Yield bug processing future commands of the current client.#10573
Fix RM_Yield bug processing future commands of the current client.#10573oranagra merged 2 commits intoredis:unstablefrom oranagra:rm_yield_protect_client
Conversation
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]>
|
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) |
There was a problem hiding this comment.
here server.current_client is always NULL I think.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think that unlocking the GIL on the Redis main thread is a miss-use of the API...
There was a problem hiding this comment.
so shall i remove the protect / unprotect calls from here?
maybe add an assertion for currecnt_client being NULL?
There was a problem hiding this comment.
i guess i'd rather leave it there even if it might be dead code..
…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
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`
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:
RM_Call. although this specific issue is gone, arguably other hooks
like keyspace notification, can do the same.
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