This is an automated email from the ASF dual-hosted git repository.
hulk pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new e749b0639 feat(script): make EVAL & FCALL exclusive if
lua-strict-key-accessing is disabled (#3155)
e749b0639 is described below
commit e749b0639bcf99f42b27ffe439442af4bb624768
Author: Twice <[email protected]>
AuthorDate: Wed Sep 3 09:49:08 2025 +0800
feat(script): make EVAL & FCALL exclusive if lua-strict-key-accessing is
disabled (#3155)
Accessing undeclared keys may lead to unexpected behavior in both
Kvrocks and Redis.
Specifically in Kvrocks, undeclared keys will not be protected by key
locks, which may face atomic issues of rocksdb IO.
If `lua-strict-key-accessing` is enabled, we can garantee that there is
no write on undeclared keys, so EVAL & FCALL in multiple threads should
be safe.
But for disabled `lua-strict-key-accessing`, it is quite dangerous if
users use undeclared keys in scripting. So we add `exclusive` flag to
EVAL, EVALSHA and FCALL commands in such case.
NOTE that *_RO commands are not affected.
---
kvrocks.conf | 9 ++++++---
src/cluster/cluster.cc | 2 +-
src/commands/cmd_function.cc | 17 ++++++++++++-----
src/commands/cmd_script.cc | 19 ++++++++++++++-----
src/commands/commander.h | 25 ++++++++++++++++++++++---
src/server/redis_connection.cc | 2 +-
src/server/server.cc | 2 +-
src/storage/scripting.cc | 11 +++++------
8 files changed, 62 insertions(+), 25 deletions(-)
diff --git a/kvrocks.conf b/kvrocks.conf
index b56934345..a4754793b 100644
--- a/kvrocks.conf
+++ b/kvrocks.conf
@@ -426,9 +426,12 @@ txn-context-enabled no
# if it tries to access keys that are not declared in
# the script's `KEYS` table or the function's `keys` argument.
#
-# Note that accessing undeclared keys may lead to unexpected behavior,
-# so this option is to ensure that scripts only access keys
-# that are explicitly declared.
+# Note that if this option is disabled, EVAL and FCALL will be
+# executed exclusively with a global lock to prevent
+# data inconsistency caused by concurrent access to undecalred keys.
+# And if it is enabled, EVAL and FCALL can be executed concurrently
+# in multiple worker threads,
+# which can improve scripting performance greatly.
#
# Default: no
lua-strict-key-accessing no
diff --git a/src/cluster/cluster.cc b/src/cluster/cluster.cc
index 62c82c01a..20fe3d362 100644
--- a/src/cluster/cluster.cc
+++ b/src/cluster/cluster.cc
@@ -896,7 +896,7 @@ Status Cluster::CanExecByMySelf(const
redis::CommandAttributes *attributes, cons
cross_slot_ok = true;
}
- uint64_t flags = attributes->GenerateFlags(cmd_tokens);
+ uint64_t flags = attributes->GenerateFlags(cmd_tokens, *srv_->GetConfig());
if (myself_ && myself_ == slots_nodes_[slot]) {
// We use central controller to manage the topology of the cluster.
diff --git a/src/commands/cmd_function.cc b/src/commands/cmd_function.cc
index 0161dd691..73fc24c4b 100644
--- a/src/commands/cmd_function.cc
+++ b/src/commands/cmd_function.cc
@@ -120,10 +120,17 @@ uint64_t GenerateFunctionFlags(uint64_t flags, const
std::vector<std::string> &a
return flags;
}
-REDIS_REGISTER_COMMANDS(Function,
- MakeCmdAttr<CommandFunction>("function", -2,
"exclusive no-script", NO_KEY,
- GenerateFunctionFlags),
- MakeCmdAttr<CommandFCall<>>("fcall", -3, "write
no-script", GetScriptEvalKeyRange),
- MakeCmdAttr<CommandFCall<true>>("fcall_ro", -3,
"read-only no-script", GetScriptEvalKeyRange));
+uint64_t GenerateFCallFlags(uint64_t flags, const std::vector<std::string> &,
const Config &config) {
+ if (!config.lua_strict_key_accessing) {
+ return flags | kCmdExclusive;
+ }
+
+ return flags;
+}
+
+REDIS_REGISTER_COMMANDS(
+ Function, MakeCmdAttr<CommandFunction>("function", -2, "exclusive
no-script", NO_KEY, GenerateFunctionFlags),
+ MakeCmdAttr<CommandFCall<>>("fcall", -3, "write no-script",
GetScriptEvalKeyRange, GenerateFCallFlags),
+ MakeCmdAttr<CommandFCall<true>>("fcall_ro", -3, "read-only no-script",
GetScriptEvalKeyRange));
} // namespace redis
diff --git a/src/commands/cmd_script.cc b/src/commands/cmd_script.cc
index 0d917d67f..ea6a37d54 100644
--- a/src/commands/cmd_script.cc
+++ b/src/commands/cmd_script.cc
@@ -125,10 +125,19 @@ uint64_t GenerateScriptFlags(uint64_t flags, const
std::vector<std::string> &arg
return flags;
}
-REDIS_REGISTER_COMMANDS(Script, MakeCmdAttr<CommandEval>("eval", -3, "write
no-script", GetScriptEvalKeyRange),
- MakeCmdAttr<CommandEvalSHA>("evalsha", -3, "write
no-script", GetScriptEvalKeyRange),
- MakeCmdAttr<CommandEvalRO>("eval_ro", -3, "read-only
no-script", GetScriptEvalKeyRange),
- MakeCmdAttr<CommandEvalSHARO>("evalsha_ro", -3,
"read-only no-script", GetScriptEvalKeyRange),
- MakeCmdAttr<CommandScript>("script", -2, "exclusive
no-script", NO_KEY, GenerateScriptFlags), )
+uint64_t GenerateEvalFlags(uint64_t flags, const std::vector<std::string> &,
const Config &config) {
+ if (!config.lua_strict_key_accessing) {
+ return flags | kCmdExclusive;
+ }
+
+ return flags;
+}
+
+REDIS_REGISTER_COMMANDS(
+ Script, MakeCmdAttr<CommandEval>("eval", -3, "write no-script",
GetScriptEvalKeyRange, GenerateEvalFlags),
+ MakeCmdAttr<CommandEvalSHA>("evalsha", -3, "write no-script",
GetScriptEvalKeyRange, GenerateEvalFlags),
+ MakeCmdAttr<CommandEvalRO>("eval_ro", -3, "read-only no-script",
GetScriptEvalKeyRange),
+ MakeCmdAttr<CommandEvalSHARO>("evalsha_ro", -3, "read-only no-script",
GetScriptEvalKeyRange),
+ MakeCmdAttr<CommandScript>("script", -2, "exclusive no-script", NO_KEY,
GenerateScriptFlags), )
} // namespace redis
diff --git a/src/commands/commander.h b/src/commands/commander.h
index a44f68505..32b54978c 100644
--- a/src/commands/commander.h
+++ b/src/commands/commander.h
@@ -33,10 +33,12 @@
#include <memory>
#include <string>
#include <thread>
+#include <type_traits>
#include <utility>
#include <vector>
#include "cluster/cluster_defs.h"
+#include "config/config.h"
#include "error_constants.h"
#include "logging.h"
#include "parse_util.h"
@@ -176,7 +178,24 @@ using CommandKeyRangeGen =
std::function<CommandKeyRange(const std::vector<std::
using CommandKeyRangeVecGen = std::function<std::vector<CommandKeyRange>(const
std::vector<std::string> &)>;
-using AdditionalFlagGen = std::function<uint64_t(uint64_t, const
std::vector<std::string> &)>;
+struct AdditionalFlagGen : std::function<uint64_t(uint64_t, const
std::vector<std::string> &, const Config &)> {
+ using BaseType = std::function<uint64_t(uint64_t, const
std::vector<std::string> &, const Config &)>;
+
+ AdditionalFlagGen() = default;
+
+ template <typename F>
+ static auto Make(F &&func) {
+ if constexpr (std::is_invocable_r_v<uint64_t, F, uint64_t, const
std::vector<std::string> &>) {
+ return BaseType(
+ [=](uint64_t flag, const std::vector<std::string> &args, const
Config &) { return func(flag, args); });
+ } else {
+ return BaseType(std::forward<F>(func));
+ }
+ }
+
+ template <typename F>
+ AdditionalFlagGen(F &&func) : BaseType(Make(std::forward<F>(func))) {} //
NOLINT
+};
struct NoKeyInThisCommand {};
static constexpr const NoKeyInThisCommand NO_KEY{};
@@ -246,9 +265,9 @@ struct CommandAttributes {
uint64_t InitialFlags() const { return flags_; }
- auto GenerateFlags(const std::vector<std::string> &args) const {
+ auto GenerateFlags(const std::vector<std::string> &args, const Config
&config) const {
uint64_t res = flags_;
- if (flag_gen_) res = flag_gen_(res, args);
+ if (flag_gen_) res = flag_gen_(res, args, config);
return res;
}
diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc
index f113e0041..06a1a4559 100644
--- a/src/server/redis_connection.cc
+++ b/src/server/redis_connection.cc
@@ -431,7 +431,7 @@ void Connection::ExecuteCommands(std::deque<CommandTokens>
*to_process_cmds) {
continue;
}
- auto cmd_flags = attributes->GenerateFlags(cmd_tokens);
+ auto cmd_flags = attributes->GenerateFlags(cmd_tokens, *config);
if (GetNamespace().empty()) {
if (!password.empty()) {
if (!(cmd_flags & kCmdAuth)) {
diff --git a/src/server/server.cc b/src/server/server.cc
index 40fc79f75..acd253745 100644
--- a/src/server/server.cc
+++ b/src/server/server.cc
@@ -2015,7 +2015,7 @@ void Server::updateAllWatchedKeys() {
}
void Server::UpdateWatchedKeysFromArgs(const std::vector<std::string> &args,
const redis::CommandAttributes &attr) {
- if ((attr.GenerateFlags(args) & redis::kCmdWrite) && watched_key_size_ > 0) {
+ if ((attr.GenerateFlags(args, *GetConfig()) & redis::kCmdWrite) &&
watched_key_size_ > 0) {
attr.ForEachKeyRange([this](const std::vector<std::string> &args,
redis::CommandKeyRange range) {
updateWatchedKeysFromRange(args, range); },
args, [this](const std::vector<std::string> &) {
updateAllWatchedKeys(); });
diff --git a/src/storage/scripting.cc b/src/storage/scripting.cc
index 357b9ae26..ff97d253c 100644
--- a/src/storage/scripting.cc
+++ b/src/storage/scripting.cc
@@ -797,7 +797,11 @@ int RedisGenericCommand(lua_State *lua, int raise_error) {
return raise_error ? RaiseError(lua) : 1;
}
- auto cmd_flags = attributes->GenerateFlags(args);
+ auto *conn = script_run_ctx->conn;
+ auto *srv = conn->GetServer();
+ Config *config = srv->GetConfig();
+
+ auto cmd_flags = attributes->GenerateFlags(args, *config);
if ((script_run_ctx->flags & ScriptFlagType::kScriptNoWrites) && !(cmd_flags
& redis::kCmdReadOnly)) {
PushError(lua, "Write commands are not allowed from read-only scripts");
@@ -810,11 +814,6 @@ int RedisGenericCommand(lua_State *lua, int raise_error) {
}
std::string cmd_name = attributes->name;
-
- auto *conn = script_run_ctx->conn;
- auto *srv = conn->GetServer();
- Config *config = srv->GetConfig();
-
cmd->SetArgs(args);
auto s = cmd->Parse();
if (!s) {
