Skip to content

Unified MULTI, LUA, and RM_Call with respect to blocking commands#8025

Merged
oranagra merged 18 commits intoredis:unstablefrom
MeirShpilraien:unified_multi_lua_rmcall_on_block_commands
Nov 17, 2020
Merged

Unified MULTI, LUA, and RM_Call with respect to blocking commands#8025
oranagra merged 18 commits intoredis:unstablefrom
MeirShpilraien:unified_multi_lua_rmcall_on_block_commands

Conversation

@MeirShpilraien
Copy link

@MeirShpilraien MeirShpilraien commented Nov 5, 2020

Blocking command should not be used with MULTI, LUA, and RM_Call. This is because, the caller, who executes the command in this context, expects a reply.

Today, LUA and MULTI have a special (and different) treatment to blocking commands:

  • LUA   - Most commands are marked with no-script flag which are checked when executing and command from LUA, commands that are not marked (like XREAD) verify that their blocking mode is not used inside LUA (by checking the CLIENT_LUA client flag).
  • MULTI - Command that is going to block, first verify that the client is not inside multi (by checking the CLIENT_MULTI client flag). If the client is inside multi, they return a result which is a match to the empty key with no timeout (for example blpop inside MULTI will act as lpop)

For modules that perform RM_Call with blocking command, the returned results type is REDISMODULE_REPLY_UNKNOWN and the caller can not really know what happened.

Disadvantages of the current state are:

  • No unified approach, LUA, MULTI, and RM_Call, each has a different treatment
  • Module can not safely execute blocking command (and get reply or error). Though It is true that modules are not like LUA or MULTI and should be smarter not to execute blocking commands on RM_Call, sometimes you want to execute a command base on client input (for example if you create a module that provides a new scripting language like javascript or python).
  • While modules (on modules command) can check for REDISMODULE_CTX_FLAGS_LUA or REDISMODULE_CTX_FLAGS_MULTI to know not to block the client, there is no way to check if the command came from another module using RM_Call. So there is no way for a module to know not to block another module RM_Call execution.

The PR suggests a way to unified the treatment for blocking clients by introducing a new CLIENT_DENY_BLOCKING client flag. On LUA, MULTI, and RM_Call the new flag turned on to signify that the client should not be blocked. A blocking command
verifies that the flag is turned off before blocking. If a blocking command sees that the CLIENT_DENY_BLOCKING flag is on, it's not blocking and return results which are matches to empty key with no timeout (as MULTI does today).

The new flag is checked on the following commands:

  • List blocking commands: BLPOP, BRPOP, BRPOPLPUSH, BLMOVE,
  • Zset blocking commands: BZPOPMIN, BZPOPMAX
  • Stream blocking commands: XREAD, XREADGROUP
  • SUBSCRIBE, PSUBSCRIBE
  • MONITOR

In addition, the new flag is turned on inside the AOF client, we do not want to block the AOF client to prevent deadlocks and commands ordering issues (and there is also an existing assert in the code that verifies it).

To keep backward compatibility on LUA, all the no-script flags on existing commands were kept untouched. In addition, a LUA special treatment on XREAD and XREADGROUP was kept.

To keep backward compatibility on MULTI (which today allows SUBSCRIBE, and PSUBSCRIBE). We added a special treatment on those commands to allow executing them on MULTI.

The only backward compatibility issue that this PR introduces is that now MONITOR is not allowed inside MULTI.

Tests were added to verify blocking commands are not blocking the client on LUA, MULTI, or RM_Call. Tests were added to verify the module can check for CLIENT_DENY_BLOCKING flag.

Related issues:

@oranagra @yossigo @guybe7 @itamarhaber let me know what you think.

Blocking command should not be used with MULTI, LUA, and RM_Call.
This is because, the caller, who execute the command in this context
expect a reply.

Today, LUA and MULTI has a special (and different) treatment to
blocking commands:
  * LUA   -Most commands are marked with no-script flag which are
           checked when executing and command from LUA, commands
           that are not marked (like X) verify that their
           blocking mode are not used inside LUA (by checking
           the CLIENT_LUA client flag).
  * MULTI -Command that are going to block, first verify that the
           client is not inside multi (by checking the CLIENT_MULTI
           client flag). If the client inside multi, they return
           a result which is a match to empty key with no timeout (for
           example blpop inside MULTI will act as lpop)

For modules that perform RM_Call with blocking command, the returned
results type is REDISMODULE_REPLY_UNKNOWN and the caller can not really
know what happened.

Disadvantages of the current state are:
  * No unified approach, LUA, MULTI, and RM_Call, each has a different treatment
  * Module can not safely execute blocking command (and get reply error).
    Though It is true that modules are not like LUA or MULTI and should be smarter
    not to execute blocking commands on RM_Call, sometimes you want to execute
    a command base on client input (for example if you create a module that provides
    a new scripting language like javascript or python).
  * While modules (on modules command) can check for REDISMODULE_CTX_FLAGS_LUA
    or REDISMODULE_CTX_FLAGS_MULTI to know not to block the client, there is no
    way to check if the command came from another module using RM_Call. So there
    is not way for a module to know not to block another module RM_Call execution.

The PR suggests a way to unified the treatment for blocking clients by introducing
a new CLIENT_DENY_BLOCKING client flag. On LUA, MULTI, and RM_Call the new flag
turned on to signify that the client should not be blocked. A blocking command
verified that the flag is turned off before blocking. If a blocking command
sees that the CLIENT_DENY_BLOCKING flag is on, it's not blocking and return results
which are matches to empty key with no timeout (as MULTI does today).

The new approach gives more freedom to execute more commands inside LUA.
In addition, it allows modules to check for the CLIENT_DENY_BLOCKING flag
before deciding to block the client on either LUA, MULTI, or RM_Call.

Tests were added to verify blocking commands are not blocked on LUA, MULTI, or RM_Call.
Tests were added to verify the module can check for CLIENT_DENY_BLOCKING flag.
Few tests were modified because now LUA is more flexible and allows for example
to call blpop (which will be executed just like lpop).
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.

if we decide to revert the behavior change on these commands in scripts, we'll need some adjustments in the tests (reverting some changes and adding others).
let's wait for more feedback before coding it.

1. Tests that verify that the blocking commands are not blocking the client if not needed.
2. LUA changes was reverted to avoid backword compatibility issues.
3. Check the new CLIENT_DENY_BLOCKING flag on subscribe and monitor.
4. Turn on the new CLIENT_DENY_BLOCKING flag on AOF connection.
1. Remove MULTI exception on monitor (which means that monitor will not be allowed insdie multi)
2. Added check for the new DENY_BLOCKING flag on PSUBSCRIBE
oranagra
oranagra previously approved these changes Nov 9, 2020
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.

please edit the PR top comment (bottom part needs an update).
please add a section with a bulleted list of behavior changes.

Moved DEBY_BLOCKIGN flag management into the exec command function to be able
to restore the previous value of the flag before the exec.
oranagra
oranagra previously approved these changes Nov 9, 2020
@oranagra
Copy link
Member

oranagra commented Nov 9, 2020

@redis/core-team please approve.
you can read the detailed top comment, basically the major decision is:

  • new REDISMODULE_CTX_FLAGS_DENY_BLOCKING flag for modules
  • MONITOR is no longer allowed inside MULTI
  • when a blocking command is called with RM_Call (and AOF), it'll behave the same as inside MULTI (non blocking)

Co-authored-by: Itamar Haber <[email protected]>
oranagra
oranagra previously approved these changes Nov 9, 2020
Copy link
Member

@itamarhaber itamarhaber left a comment

Choose a reason for hiding this comment

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

Some grammar/typos/style comments

src/server.c Outdated
void monitorCommand(client *c) {
if (c->flags & CLIENT_DENY_BLOCKING) {
/**
* A Client that has CLIENT_DENY_BLOCKING flag on
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* A Client that has CLIENT_DENY_BLOCKING flag on
* A client that has CLIENT_DENY_BLOCKING flag on

Co-authored-by: Itamar Haber <[email protected]>
Meir Shpilraien (Spielrein) and others added 11 commits November 9, 2020 17:33
Co-authored-by: Itamar Haber <[email protected]>
Co-authored-by: Itamar Haber <[email protected]>
Co-authored-by: Itamar Haber <[email protected]>
Co-authored-by: Itamar Haber <[email protected]>
Co-authored-by: Itamar Haber <[email protected]>
Co-authored-by: Itamar Haber <[email protected]>
Co-authored-by: Itamar Haber <[email protected]>
Co-authored-by: Itamar Haber <[email protected]>
Co-authored-by: Itamar Haber <[email protected]>
Co-authored-by: Itamar Haber <[email protected]>
@oranagra oranagra added 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 state:major-decision Requires core team consensus labels Nov 9, 2020
@madolson
Copy link
Contributor

API change looks good to me, but I want to dive into the code before approving.

@oranagra oranagra merged commit d87a0d0 into redis:unstable Nov 17, 2020
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Nov 19, 2020
…dis#8025)

Blocking command should not be used with MULTI, LUA, and RM_Call. This is because,
the caller, who executes the command in this context, expects a reply.

Today, LUA and MULTI have a special (and different) treatment to blocking commands:

LUA   - Most commands are marked with no-script flag which are checked when executing
and command from LUA, commands that are not marked (like XREAD) verify that their
blocking mode is not used inside LUA (by checking the CLIENT_LUA client flag).
MULTI - Command that is going to block, first verify that the client is not inside
multi (by checking the CLIENT_MULTI client flag). If the client is inside multi, they
return a result which is a match to the empty key with no timeout (for example blpop
inside MULTI will act as lpop)
For modules that perform RM_Call with blocking command, the returned results type is
REDISMODULE_REPLY_UNKNOWN and the caller can not really know what happened.

Disadvantages of the current state are:

No unified approach, LUA, MULTI, and RM_Call, each has a different treatment
Module can not safely execute blocking command (and get reply or error).
Though It is true that modules are not like LUA or MULTI and should be smarter not
to execute blocking commands on RM_Call, sometimes you want to execute a command base
on client input (for example if you create a module that provides a new scripting
language like javascript or python).
While modules (on modules command) can check for REDISMODULE_CTX_FLAGS_LUA or
REDISMODULE_CTX_FLAGS_MULTI to know not to block the client, there is no way to
check if the command came from another module using RM_Call. So there is no way
for a module to know not to block another module RM_Call execution.

This commit adds a way to unify the treatment for blocking clients by introducing
a new CLIENT_DENY_BLOCKING client flag. On LUA, MULTI, and RM_Call the new flag
turned on to signify that the client should not be blocked. A blocking command
verifies that the flag is turned off before blocking. If a blocking command sees
that the CLIENT_DENY_BLOCKING flag is on, it's not blocking and return results
which are matches to empty key with no timeout (as MULTI does today).

The new flag is checked on the following commands:

List blocking commands: BLPOP, BRPOP, BRPOPLPUSH, BLMOVE,
Zset blocking commands: BZPOPMIN, BZPOPMAX
Stream blocking commands: XREAD, XREADGROUP
SUBSCRIBE, PSUBSCRIBE, MONITOR
In addition, the new flag is turned on inside the AOF client, we do not want to
block the AOF client to prevent deadlocks and commands ordering issues (and there
is also an existing assert in the code that verifies it).

To keep backward compatibility on LUA, all the no-script flags on existing commands
were kept untouched. In addition, a LUA special treatment on XREAD and XREADGROUP was kept.

To keep backward compatibility on MULTI (which today allows SUBSCRIBE, and PSUBSCRIBE).
We added a special treatment on those commands to allow executing them on MULTI.

The only backward compatibility issue that this PR introduces is that now MONITOR
is not allowed inside MULTI.

Tests were added to verify blocking commands are not blocking the client on LUA, MULTI,
or RM_Call. Tests were added to verify the module can check for CLIENT_DENY_BLOCKING flag.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: Itamar Haber <[email protected]>
@oranagra oranagra mentioned this pull request Jan 13, 2021
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:major-decision Requires core team consensus 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.

4 participants