-
Notifications
You must be signed in to change notification settings - Fork 24.4k
Support for RM_Call on blocking commands #11568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fix redis#7992, allow running blocking commands from within a module using `RM_Call`. Today, when `RM_Call` is used, the fake client that is used to run command is marked with `CLIENT_DENY_BLOCKING` flag. This flags tells the command that it is not allowed to block the client and in case it needs to block, it must fallback to some alternative (either return error or perform some default behavior). For example, `BLPOP` fallback to simple `LPOP` if it is not allow to block. All the commands must respect the `CLIENT_DENY_BLOCKING` flag (including module commands). When the command invocation finished, Redis asserts that the client was not blocked. This PR introduces the ability to call blocking command using `RM_Call` by passing a callback that will be called when the client will get unblocked. The callback can be passed to `RM_Call` using a new format specifier argument, `K`, follow by the callback and a private data (`void*`). If the new specified is given to `RM_Call`, Redis knows that its OK to block and in such case it performs the following: 1. **Not** setting `CLIENT_DENY_BLOCKING` flag on the fake client so the command will know it is ok to block the client. 2. When command invocation finishes, Redis checks if the client was blocked: 1. If not, no changes to the code flow is done and the reply is simply return. 2. If case the client was blocked, Redis set the callback and the private data on the fake client So it will be possible to call it when the client gets unblocked. Then Redis returns `NULL` reply (with no `errno` set) indicating the client was blocked. When clients gets unblock, it eventually reaches `processUnblockedClients` function. This is where we check if the client is a fake module client and if it is, we call the unblock callback instead of performing the usual unblock operations. ### Calling blocking commands while running on script mode (`S`) `RM_Call` script mode (`S`) was introduce on redis#10372. It is used for usecases where the command that was invoked on `RM_Call` comes from a user input and we want to make sure the user will not run dangerous commands like `shutdown`. Some command, such as `BLPOP`, are marked with `NO_SCRIPT` flag, which means they will not be allowed on script mode. Those commands are marked with `NO_SCRIPT` just because they are blocking commands and not because they are dangerous. Now that we can run blocking commands on RM_Call, there is no real reason not to allow such commands on script mode. The underline problem is that the `NO_SCRIPT` flag is abused to also mark some of the blocking commands (notice that those commands knows not to block the client if they are not allowed to, and have a fallback logic to such cases. So even if those commands was not makred with `NO_SCRIPT` flag, it would not harm Redis, and today we can already run those commands within multi exec). In addition, not all blocking commands are marked with `NO_SCRIPT` flag, for example `blmpop` are not marked and can run from within a script. Those facts shows that there are some ambiguity about the meaning of the `NO_SCRIPT` flag, and its not fully clear where it should be use. The PR suggest that blocking commands should not be marked with `NO_SCRIPT` flag, those commands should handle `CLIENT_DENY_BLOCKING` flag and only block when it's safe (like they already does today). To achieve that, the PR removes the `NO_SCRIPT` flag from the following commands: * `blmove` * `blpop` * `brpop` * `brpoplpush` * `bzpopmax` * `bzpopmin` * `wait` This might be considered a breaking change as now, on scripts, instead of getting `command is not allowed from script` error, the user will get some fallback behavior base on the command implementation. That said, the change matches the behavior of scripts and multi exec with respect to those commands and allow running them on `RM_Call` even when script mode is used. ### TODO * Accept/Decline the suggest API (using format specifier argument `K`) * Decide if `processUnblockedClients` is the right place to call the unblock callback * Decide if the changes to `BLPOP` like commands are acceptable or is it a breaking change.
|
i didn't look at the code yet, but from the description, the one thing i don't like is the API. regarding the NO_SCRIPT flag, i don't think that it should be a problem removing the flag from the blocking commands, existing scripts couldn't use these commands anyway, so they won't be affected, but we have to take into consideration few other external aspects:
one interesting thing is that the BLOCKING flag (which modules can also use when they register commands) is completely new, and likely not in use yet, that probably means we can't use it. |
Sure, this is just a suggestion but totally debatable. We can have a new API,
The
Well, before, when call those list of commands from script, it would have count as an error. Now it will not be counted as error. But as you said, there was not point of calling those commands from script so not sure it is a breaking change.
Right, currently modules have to respect the |
oranagra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looks good.
haven't reviewed the tests yet.
1. Introduce promise call reply object that will be return by RM_Call if the command was blocked. 2. Allow setting the on unblocked callbacks on the promise reply object.
|
triggered valgrind https://github.com/redis/redis/actions/runs/4216703412 |
Co-authored-by: Oran Agra <oran@redislabs.com>
|
@oranagra fixed the comments except for #11568 (comment) which I am waiting for input. |
ranshid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MeirShpilraien what will happen in case of pipelined module command which will attempt to trigger RM_Call with blocking command?
basically we could have more commands pending in the input buffer but I am not sure we have a way to turn back and process the input buffer (did I miss something?)
@ranshid I am not sure I am follow, each blocking RM_Call is invoke on a separate fake client. Maybe I do not understand the problem you refer to? |
well correct me if I am wrong: for regular client blocking via modules, the executing client is blocked and then later when it is unblocked by the module is queued on 2 lists:
In your case if the module command is currently executed and is spinning a fake client which is then unblocked. the fake client is reprocessed and execute the module continuation callback. but what will cause the processing of what is left in the executing client inputBuffer? (again I might be missing something, but wanted to raise that anyway) |
Co-authored-by: Oran Agra <oran@redislabs.com>
The [blocking `RM_Call`](redis/redis#11568) was introduce on Redis 7.2. The idea is to give a module writter the ability to perform blocking commands like `BLPOP` using `RM_Call`. This PR adds this functionallity to `redismodule-rs`. In order to be able to invoke blocking commands, the user need to use `call_blocking` instead of `call` or `call_ext`. `call_blocking` will return an enum that can either be a regular reply (like `call_ext` returns) or it can be a `FutureCallReply`. The `FutureCallReply` can be used to set `on_done_handler` that will be called when the command gets unblock. The `on_done_handler` gets the command reply as an intput. The `FutureCallReply` not outlive the `Context` that was used to invoke the blocking command. This is because the `on_done_handler` must be set before the Redis GIL is released. This restriction forces it. After setting the unblock handler, the user will get a `FutureHandler` object that can be use to abort the command invocation. The abort is done base on a best effort approach. And important details is that the `FutureHandler` must be freed when Redis GIL is held, this is why we introduce a `dispose` function (that gets a lock indicator) and not counting on `Drop` implementation. A simple usage example: ```rust fn call_blocking(ctx: &Context, _: Vec<RedisString>) -> RedisResult { // create blocking call options let call_options = CallOptionsBuilder::new().build_blocking(); // call the blocking command let res = ctx.call_blocking("blpop", &call_options, &["list", "1"]); // check the reply, if its a future, block the client until the // future is resolved. match res { PromiseCallReply::Resolved(r) => r.map_or_else(|e| Err(e.into()), |v| Ok((&v).into())), PromiseCallReply::Future(f) => { let blocked_client = ctx.block_client(); f.set_unblock_handler(move |_ctx, reply| { let thread_ctx = ThreadSafeContext::with_blocked_client(blocked_client); thread_ctx.reply(reply.map_or_else(|e| Err(e.into()), |v| Ok((&v).into()))); }); Ok(RedisValue::NoReply) } } } ``` **Notice** a nice possible improvement would be to integrate this feature with rust async await. We leave this for future PR.
The [blocking `RM_Call`](redis/redis#11568) was introduce on Redis 7.2. The idea is to give a module writer the ability to perform blocking commands like `BLPOP` using `RM_Call`. This PR adds this functionality to `redismodule-rs`. In order to be able to invoke blocking commands, the user need to use `call_blocking` instead of `call` or `call_ext`. `call_blocking` will return an enum that can either be a regular reply (like `call_ext` returns) or it can be a `FutureCallReply`. The `FutureCallReply` can be used to set `on_done_handler` that will be called when the command gets unblock. The `on_done_handler` gets the command reply as an input. The `FutureCallReply` not outlive the `Context` that was used to invoke the blocking command. This is because the `on_done_handler` must be set before the Redis GIL is released. This restriction forces it. After setting the unblock handler, the user will get a `FutureHandler` object that can be use to abort the command invocation. The abort is done base on a best effort approach. And important details is that the `FutureHandler` must be freed when Redis GIL is held, this is why we introduce a `dispose` function (that gets a lock indicator) and not counting on `Drop` implementation. A simple usage example: ```rust fn call_blocking(ctx: &Context, _: Vec<RedisString>) -> RedisResult { // create blocking call options let call_options = CallOptionsBuilder::new().build_blocking(); // call the blocking command let res = ctx.call_blocking("blpop", &call_options, &["list", "1"]); // check the reply, if its a future, block the client until the // future is resolved. match res { PromiseCallReply::Resolved(r) => r.map_or_else(|e| Err(e.into()), |v| Ok((&v).into())), PromiseCallReply::Future(f) => { let blocked_client = ctx.block_client(); f.set_unblock_handler(move |_ctx, reply| { let thread_ctx = ThreadSafeContext::with_blocked_client(blocked_client); thread_ctx.reply(reply.map_or_else(|e| Err(e.into()), |v| Ok((&v).into()))); }); Ok(RedisValue::NoReply) } } } ``` **Notice** a nice possible improvement would be to integrate this feature with rust async await. We leave this for future PR. **Notice** The PR also updates the `redismodule.h` file.
blocking RM_Call was introduced on: #11568, It allows a module to perform blocking commands and get the reply asynchronously.If the command gets block, a special promise CallReply is returned that allow to set the unblock handler. The unblock handler will be called when the command invocation finish and it gets, as input, the command real reply. The issue was that the real CallReply was created using a stack allocated RedisModuleCtx which is no longer available after the unblock handler finishes. So if the module keeps the CallReply after the unblock handler finished, the CallReply holds a pointer to invalid memory and will try to access it when the CallReply will be released. The solution is to create the CallReply with a NULL context to make it totally detached and can be freed freely when the module wants. Test was added to cover this case, running the test with valgrind before the fix shows the use after free error. With the fix, there are no valgrind errors. unrelated: adding a missing `$rd close` in many tests in that file.
In #11568 we removed the NOSCRIPT flag from commands and keep the BLOCKING flag. Aiming to allow them in scripts and let them implicitly behave in the non-blocking way. In that sense, the old behavior was to allow LPOP and reject BLPOP, and the new behavior, is to allow BLPOP too, and fail it only in case it ends up blocking. So likewise, so far we allowed XREAD and rejected XREAD BLOCK, and we will now allow that too, and only reject it if it ends up blocking.
In #11568 we removed the NOSCRIPT flag from commands, e.g. removing NOSCRIPT flag from WAIT. Aiming to allow them in scripts and let them implicitly behave in the non-blocking way. This PR remove NOSCRIPT flag from WAITAOF just like WAIT (to be symmetrical)). And this PR also add BLOCKING flag for WAIT and WAITAOF.
In redis#11568 we removed the NOSCRIPT flag from commands, e.g. removing NOSCRIPT flag from WAIT. Aiming to allow them in scripts and let them implicitly behave in the non-blocking way. This PR remove NOSCRIPT flag from WAITAOF just like WAIT (to be symmetrical)). And this PR also add BLOCKING flag for WAIT and WAITAOF.
In redis#11568 we removed the NOSCRIPT flag from commands, e.g. removing NOSCRIPT flag from WAIT. Aiming to allow them in scripts and let them implicitly behave in the non-blocking way. This PR remove NOSCRIPT flag from WAITAOF just like WAIT (to be symmetrical)). And this PR also add BLOCKING flag for WAIT and WAITAOF.
Fix #7992, allow running blocking commands from within a module using
RM_Call.Today, when
RM_Callis used, the fake client that is used to run command is marked withCLIENT_DENY_BLOCKINGflag. This flag tells the command that it is not allowed to block the client and in case it needs to block, it must fallback to some alternative (either return error or perform some default behaviour). For example,BLPOPfallback to simpleLPOPif it is not allowed to block.All the commands must respect the
CLIENT_DENY_BLOCKINGflag (including module commands). When the command invocation finished, Redis asserts that the client was not blocked.This PR introduces the ability to call blocking command using
RM_Callby passing a callback that will be called when the client will get unblocked. In order to do that, the user must explicitly say that he allow to perform blocking command by passing a new format specifier argument,K, to theRM_Callfunction. This new flag will tell Redis that it is allow to run blocking command and block the client. In case the command got blocked, Redis will return a new type of call reply (REDISMODULE_REPLY_PROMISE). This call reply indicates that the command got blocked and the user can set the on_unblocked handler usingRM_CallReplyPromiseSetUnblockHandler.When clients gets unblocked, it eventually reaches
processUnblockedClientsfunction. This is where we check if the client is a fake module client and if it is, we call the unblock callback instead of performing the usual unblock operations.Notice:
RM_CallReplyPromiseSetUnblockHandlermust be called atomically along side the command invocation (without releasing the Redis lock in between). In addition, unlike other CallReply types, the promise call reply must be released by the module when the Redis GIL is acquired.The module can abort the execution on the blocking command (if it was not yet executed) using
RM_CallReplyPromiseAbort. the API will returnREDISMODULE_OKon success andREDISMODULE_ERRif the operation is already executed. Notice that in case of misbehave module, Abort might finished successfully but the operation will not really be aborted. This can only happened if the module do not respect the disconnect callback of the blocked client.For pure Redis commands this can not happened.
Atomicity Guarantees
The API promise that the unblock handler will run atomically as an execution unit. This means that all the operation performed on the unblock handler will be wrapped with a multi exec transaction when replicated to the replica and AOF. The API do not grantee any other atomicity properties such as when the unblock handler will be called. This gives us the flexibility to strengthen the grantees (or not) in the future if we will decide that we need a better guarantees.
That said, the implementation does provide a better guarantees when performing pure Redis blocking command like
BLPOP. In this case the unblock handler will run atomically with the operation that got unblocked (for example, in case ofBLPOP, the unblock handler will run atomically with theLPOPoperation that run when the command got unblocked). This is an implementation detail that might be change in the future and the module writer should not count on that.Calling blocking commands while running on script mode (
S)RM_Callscript mode (S) was introduce on #10372. It is used for usecases where the command that was invoked onRM_Callcomes from a user input and we want to make sure the user will not run dangerous commands likeshutdown. Some command, such asBLPOP, are marked withNO_SCRIPTflag, which means they will not be allowed on script mode. Those commands are marked withNO_SCRIPTjust because they are blocking commands and not because they are dangerous. Now that we can run blocking commands on RM_Call, there is no real reason not to allow such commands on script mode.The underline problem is that the
NO_SCRIPTflag is abused to also mark some of the blocking commands (notice that those commands know not to block the client if it is not allowed to do so, and have a fallback logic to such cases. So even if those commands were not marked withNO_SCRIPTflag, it would not harm Redis, and today we can already run those commands within multi exec).In addition, not all blocking commands are marked with
NO_SCRIPTflag, for exampleblmpopare not marked and can run from within a script.Those facts shows that there are some ambiguity about the meaning of the
NO_SCRIPTflag, and its not fully clear where it should be use.The PR suggest that blocking commands should not be marked with
NO_SCRIPTflag, those commands should handleCLIENT_DENY_BLOCKINGflag and only block when it's safe (like they already does today). To achieve that, the PR removes theNO_SCRIPTflag from the following commands:blmoveblpopbrpopbrpoplpushbzpopmaxbzpopminwaitThis might be considered a breaking change as now, on scripts, instead of getting
command is not allowed from scripterror, the user will get some fallback behaviour base on the command implementation. That said, the change matches the behaviour of scripts and multi exec with respect to those commands and allow running them onRM_Calleven when script mode is used.Additional RedisModule API and changes
RM_BlockClientSetPrivateData- Set private data on the blocked client without the need to unblock the client. This allows up to set the promise CallReply as the private data of the blocked client and abort it if the client gets disconnected.RM_BlockClientGetPrivateData- Return the current private data set on a blocked client. We need it so we will have access to this private data on the disconnect callback.TODO
K) - API was changed and top comment was updated.processUnblockedClientsis the right place to call the unblock callbackBLPOPlike commands are acceptable or is it a breaking change.