Skip to content

Conversation

@MeirShpilraien
Copy link

@MeirShpilraien MeirShpilraien commented Dec 1, 2022

Fix #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 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, BLPOP fallback to simple LPOP if it is not allowed 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. 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 the RM_Call function. 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 using RM_CallReplyPromiseSetUnblockHandler.

When clients gets unblocked, 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.

Notice: RM_CallReplyPromiseSetUnblockHandler must 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 return REDISMODULE_OK on success and REDISMODULE_ERR if 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 of BLPOP, the unblock handler will run atomically with the LPOP operation 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_Call script mode (S) was introduce on #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 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 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 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 on RM_Call even 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.
  • On RM_Call, the returned reply will be added to the auto memory context only if auto memory is enabled, this allows us to keep the call reply for longer time then the context lifetime and does not force an unneeded borrow relationship between the CallReply and the RedisModuleContext.

TODO

  • Accept/Decline the suggest API (using format specifier argument K) - API was changed and top comment was updated.
  • 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.
  • Test role change while having a blocked RM_Call.
  • replication tests
  • key space notification tests

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.
@MeirShpilraien MeirShpilraien marked this pull request as draft December 1, 2022 16:40
@MeirShpilraien MeirShpilraien requested review from enjoy-binbin, guybe7 and oranagra and removed request for enjoy-binbin December 1, 2022 16:41
@oranagra
Copy link
Member

oranagra commented Dec 1, 2022

i didn't look at the code yet, but from the description, the one thing i don't like is the API.
the fact we pass the callback in the varargs means we have no type checking, and i'm also not happy with the NULL return and the dependence of errno.

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:

  1. existing modules that exposed blocking commands to redis and may have also set the NO_SCRIPT flag.
  2. external clients introspecting commands with COMMAND INFO and how it affects them.

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.
however, the NO_MULTI flag isn't exposed to modules to this day, so i guess modules couldn't have relied on setting NO_SCRIPT, and had to react to CLIENT_DENY_BLOCKING anyway.

@MeirShpilraien
Copy link
Author

i didn't look at the code yet, but from the description, the one thing i don't like is the API.
the fact we pass the callback in the varargs means we have no type checking, and i'm also not happy with the NULL return and the dependence of errno.

Sure, this is just a suggestion but totally debatable. We can have a new API, RM_AsyncCall that gets the callback as one on the arguments, this will solve the type checking. The new function can also have an out param indicating whether it was blocked or not. For the rest, this new function can behave the same as RM_Call. Let me know how it sounds and I also like to hear @yossigo @itamarhaber @guybe7 and others about this.

existing modules that exposed blocking commands to redis and may have also set the NO_SCRIPT flag

The NO_SCRIPT flag remains the same, the command will not be able to be call inside a script or in RM_Call script mode. The change is on this list of those specific commands where we remove this flag.

external clients introspecting commands with COMMAND INFO and how it affects them.

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.

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.
however, the NO_MULTI flag isn't exposed to modules to this day, so i guess modules couldn't have relied on setting NO_SCRIPT, and had to react to CLIENT_DENY_BLOCKING anyway.

Right, currently modules have to respect the CLIENT_DENY_BLOCKING flag.

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.

generally looks good.
haven't reviewed the tests yet.

meir added 4 commits February 19, 2023 09:37
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.
@MeirShpilraien MeirShpilraien marked this pull request as ready for review February 19, 2023 10:55
@oranagra
Copy link
Member

Meir Shpilraien (Spielrein) and others added 2 commits February 20, 2023 10:30
Co-authored-by: Oran Agra <oran@redislabs.com>
@MeirShpilraien
Copy link
Author

@oranagra fixed the comments except for #11568 (comment) which I am waiting for input.

Copy link
Contributor

@ranshid ranshid left a 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?)

@MeirShpilraien
Copy link
Author

@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?

@ranshid
Copy link
Contributor

ranshid commented Feb 20, 2023

@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:

  • the moduleUnblockedClients - which is processed in the before sleep right before processing unblocked clients
  • the unblockedClients list which is processed after the moduleHandleBlockedClients() and is also processing what is left to be processed in the inputBuffer.

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)

@oranagra
Copy link
Member

Meir Shpilraien (Spielrein) and others added 4 commits March 16, 2023 12:29
@oranagra oranagra merged commit d0da0a6 into redis:unstable Mar 16, 2023
MeirShpilraien pushed a commit to RedisLabsModules/redismodule-rs that referenced this pull request May 18, 2023
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.
MeirShpilraien pushed a commit to RedisLabsModules/redismodule-rs that referenced this pull request Jun 6, 2023
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.
oranagra pushed a commit that referenced this pull request Jun 25, 2023
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.
oranagra pushed a commit that referenced this pull request Oct 12, 2023
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.
oranagra pushed a commit that referenced this pull request Jan 23, 2024
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.
roggervalf pushed a commit to roggervalf/redis that referenced this pull request Feb 11, 2024
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.
funny-dog pushed a commit to funny-dog/redis that referenced this pull request Sep 17, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

RedisModule API for async RM_Call

7 participants