This is an automated email from the ASF dual-hosted git repository.
twice 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 63960f9b3 fix(function): reset lua states from all workers in FUNCTION
DELETE (#3115)
63960f9b3 is described below
commit 63960f9b32b75b257515090a635d5ebe7ed01619
Author: Twice <[email protected]>
AuthorDate: Sat Aug 16 11:02:04 2025 +0800
fix(function): reset lua states from all workers in FUNCTION DELETE (#3115)
---
src/storage/scripting.cc | 44 ++++++++++++++++------------
src/storage/scripting.h | 4 +--
tests/gocase/unit/scripting/function_test.go | 16 ++++++++++
3 files changed, 44 insertions(+), 20 deletions(-)
diff --git a/src/storage/scripting.cc b/src/storage/scripting.cc
index 93a9463d2..d4b9a2546 100644
--- a/src/storage/scripting.cc
+++ b/src/storage/scripting.cc
@@ -282,7 +282,7 @@ int RedisRegisterFunction(lua_State *lua) {
}
Status FunctionLoad(redis::Connection *conn, engine::Context *ctx, const
std::string &script, bool need_to_store,
- bool replace, [[maybe_unused]] std::string *lib_name, bool
read_only) {
+ bool replace, [[maybe_unused]] std::string *lib_name) {
std::string first_line, lua_code;
if (auto pos = script.find('\n'); pos != std::string::npos) {
first_line = script.substr(0, pos);
@@ -293,10 +293,7 @@ Status FunctionLoad(redis::Connection *conn,
engine::Context *ctx, const std::st
const auto libname = GET_OR_RET(ExtractLibNameFromShebang(first_line));
- auto srv = conn->GetServer();
- auto lua = conn->Owner()->Lua();
-
- if (FunctionIsLibExist(conn, ctx, libname, need_to_store, read_only)) {
+ if (FunctionIsLibExist(conn, ctx, libname, need_to_store)) {
if (!replace) {
return {Status::NotOK, "library already exists, please specify REPLACE
to force load"};
}
@@ -304,10 +301,17 @@ Status FunctionLoad(redis::Connection *conn,
engine::Context *ctx, const std::st
if (!s) return s;
}
+ auto srv = conn->GetServer();
+ auto lua = conn->Owner()->Lua();
+
ScriptRunCtx script_run_ctx;
script_run_ctx.conn = conn;
script_run_ctx.ctx = ctx;
- script_run_ctx.flags = read_only ? ScriptFlagType::kScriptNoWrites : 0;
+ // in FunctionLoad(), the script MUST be read-only,
+ // because this function can be called multiple times
+ // for one script from multiple threads,
+ // and it is dangerous and unexpected to allow writes here.
+ script_run_ctx.flags = ScriptFlagType::kScriptNoWrites;
SaveOnRegistry(lua, REGISTRY_SCRIPT_RUN_CTX_NAME, &script_run_ctx);
@@ -339,7 +343,7 @@ Status FunctionLoad(redis::Connection *conn,
engine::Context *ctx, const std::st
RemoveFromRegistry(lua, REGISTRY_SCRIPT_RUN_CTX_NAME);
- if (!FunctionIsLibExist(conn, ctx, libname, false, read_only)) {
+ if (!FunctionIsLibExist(conn, ctx, libname, false)) {
return {Status::NotOK, "Please register some function in FUNCTION LOAD"};
}
@@ -347,7 +351,7 @@ Status FunctionLoad(redis::Connection *conn,
engine::Context *ctx, const std::st
}
bool FunctionIsLibExist(redis::Connection *conn, engine::Context *ctx, const
std::string &libname,
- bool need_check_storage, bool read_only) {
+ bool need_check_storage) {
auto srv = conn->GetServer();
auto lua = conn->Owner()->Lua();
@@ -374,7 +378,7 @@ bool FunctionIsLibExist(redis::Connection *conn,
engine::Context *ctx, const std
if (!s) return false;
std::string lib_name;
- s = FunctionLoad(conn, ctx, code, false, false, &lib_name, read_only);
+ s = FunctionLoad(conn, ctx, code, false, false, &lib_name);
return static_cast<bool>(s);
}
@@ -399,7 +403,7 @@ Status FunctionCall(redis::Connection *conn,
engine::Context *ctx, const std::st
std::string libcode;
s = srv->FunctionGetCode(libname, &libcode);
if (!s) return s;
- s = FunctionLoad(conn, ctx, libcode, false, false, &libname, read_only);
+ s = FunctionLoad(conn, ctx, libcode, false, false, &libname);
if (!s) return s;
lua_getglobal(lua, (REDIS_LUA_REGISTER_FUNC_PREFIX + name).c_str());
@@ -568,6 +572,11 @@ Status FunctionListLib(redis::Connection *conn, const
std::string &libname, std:
Status FunctionDelete(engine::Context &ctx, redis::Connection *conn, const
std::string &name) {
auto lua = conn->Owner()->Lua();
+ // load the library into this lua state
+ if (!FunctionIsLibExist(conn, &ctx, name, true)) {
+ return {Status::NotOK, "the library does not exist in lua environment"};
+ }
+
lua_getglobal(lua, REDIS_FUNCTION_LIBRARIES);
if (lua_isnil(lua, -1)) {
lua_pop(lua, 1);
@@ -586,22 +595,21 @@ Status FunctionDelete(engine::Context &ctx,
redis::Connection *conn, const std::
for (size_t i = 1; i <= lua_objlen(lua, -1); ++i) {
lua_rawgeti(lua, -1, static_cast<int>(i));
std::string func = lua_tostring(lua, -1);
- lua_pushnil(lua);
- lua_setglobal(lua, (REDIS_LUA_REGISTER_FUNC_PREFIX + func).c_str());
- lua_pushnil(lua);
- lua_setglobal(lua, (REDIS_LUA_REGISTER_FUNC_FLAGS_PREFIX + func).c_str());
auto _ = storage->Delete(ctx, rocksdb::WriteOptions(), cf,
engine::kLuaFuncLibPrefix + func);
lua_pop(lua, 1);
}
- lua_pop(lua, 1);
- lua_pushnil(lua);
- lua_setfield(lua, -2, name.c_str());
- lua_pop(lua, 1);
+ lua_pop(lua, 2);
auto s = storage->Delete(ctx, rocksdb::WriteOptions(), cf,
engine::kLuaLibCodePrefix + name);
if (!s.ok()) return {Status::NotOK, s.ToString()};
+ // reset all lua context from all workers
+ // to ensure the library is removed from all lua states
+ // TODO: optimize it to only delete this library
+ // instead of resetting all libraries
+ conn->GetServer()->ScriptReset();
+
return Status::OK();
}
diff --git a/src/storage/scripting.h b/src/storage/scripting.h
index f41cf6e9a..86130aac4 100644
--- a/src/storage/scripting.h
+++ b/src/storage/scripting.h
@@ -70,7 +70,7 @@ Status EvalGenericCommand(redis::Connection *conn,
engine::Context *ctx, const s
bool ScriptExists(lua_State *lua, const std::string &sha);
Status FunctionLoad(redis::Connection *conn, engine::Context *ctx, const
std::string &script, bool need_to_store,
- bool replace, std::string *lib_name, bool read_only =
false);
+ bool replace, std::string *lib_name);
Status FunctionCall(redis::Connection *conn, engine::Context *ctx, const
std::string &name,
const std::vector<std::string> &keys, const
std::vector<std::string> &argv, std::string *output,
bool read_only = false);
@@ -81,7 +81,7 @@ Status FunctionListFunc(Server *srv, const redis::Connection
*conn, engine::Cont
Status FunctionListLib(redis::Connection *conn, const std::string &libname,
std::string *output);
Status FunctionDelete(engine::Context &ctx, redis::Connection *conn, const
std::string &name);
bool FunctionIsLibExist(redis::Connection *conn, engine::Context *ctx, const
std::string &libname,
- bool need_check_storage = true, bool read_only =
false);
+ bool need_check_storage = true);
const char *RedisProtocolToLuaType(lua_State *lua, const char *reply);
const char *RedisProtocolToLuaTypeInt(lua_State *lua, const char *reply);
diff --git a/tests/gocase/unit/scripting/function_test.go
b/tests/gocase/unit/scripting/function_test.go
index 43043a614..f58934952 100644
--- a/tests/gocase/unit/scripting/function_test.go
+++ b/tests/gocase/unit/scripting/function_test.go
@@ -289,6 +289,22 @@ var testFunctions = func(t *testing.T, config
util.KvrocksServerConfigs) {
Name: "mylib3", Engine: "lua", Functions:
[]interface{}{"myget", "myset"},
}, decodeListLibResult(t, r))
})
+
+ t.Run("FUNCTION DELETE from multiple clients", func(t *testing.T) {
+ // we expect that rdb2 is accepted from another server thread,
+ // but it may not (and that's fine)
+ rdb2 := srv.NewClient()
+ defer func() { require.NoError(t, rdb2.Close()) }()
+
+ require.Equal(t, rdb.Do(ctx, "FCALL", "reverse", 0,
"abc").Val(), "cba")
+
+ require.NoError(t, rdb2.Do(ctx, "FUNCTION", "DELETE",
"mylib1").Err())
+ util.ErrorRegexp(t, rdb.Do(ctx, "FCALL", "reverse", 0,
"abc").Err(), ".*No such function name.*")
+ util.ErrorRegexp(t, rdb2.Do(ctx, "FCALL", "reverse", 0,
"abc").Err(), ".*No such function name.*")
+
+ require.NoError(t, rdb.Do(ctx, "FCALL", "myset", 1,
"func-test-tmp-a", 1).Err())
+ require.NoError(t, rdb2.Do(ctx, "FCALL", "myget", 1,
"func-test-tmp-a").Err())
+ })
}
func TestFunctionScriptFlags(t *testing.T) {
