Skip to content
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

feat(script): allow blocking commands in scripting #2740

Merged
merged 3 commits into from
Jan 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/commands/blocking_commander.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class BlockingCommander : public Commander,
// to start the blocking process
// usually put to the end of the Execute method
Status StartBlocking(int64_t timeout, std::string *output) {
if (conn_->IsInExec()) {
if (conn_->IsInExec() || conn_->IsInScript()) {
*output = NoopReply(conn_);
return Status::OK(); // no blocking in multi-exec
}
Expand Down
6 changes: 6 additions & 0 deletions src/server/redis_connection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,12 @@ void Connection::ExecuteCommands(std::deque<CommandTokens> *to_process_cmds) {
continue;
}

ScopeExit in_script_exit{[this] { in_script_ = false; }, false};
if (attributes->category == CommandCategory::Script || attributes->category == CommandCategory::Function) {
in_script_ = true;
in_script_exit.Enable();
}

SetLastCmd(cmd_name);
{
std::optional<MultiLockGuard> guard;
Expand Down
2 changes: 2 additions & 0 deletions src/server/redis_connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ class Connection : public EvbufCallbackBase<Connection> {
// Multi exec
void SetInExec() { in_exec_ = true; }
bool IsInExec() const { return in_exec_; }
bool IsInScript() const { return in_script_; }
bool IsMultiError() const { return multi_error_; }
void ResetMultiExec();
std::deque<redis::CommandTokens> *GetMultiExecCommands() { return &multi_cmds_; }
Expand Down Expand Up @@ -210,6 +211,7 @@ class Connection : public EvbufCallbackBase<Connection> {
bool multi_error_ = false;
std::atomic<bool> is_running_ = false;
std::deque<redis::CommandTokens> multi_cmds_;
bool in_script_ = false;

bool importing_ = false;
RESP protocol_version_ = RESP::v2;
Expand Down
8 changes: 1 addition & 7 deletions src/storage/scripting.cc
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,7 @@ int RedisGenericCommand(lua_State *lua, int raise_error) {

auto attributes = cmd->GetAttributes();
if (!attributes->CheckArity(argc)) {
PushError(lua, "Wrong number of args calling Redis command From Lua script");
PushError(lua, "Wrong number of args while calling Redis command from Lua script");
return raise_error ? RaiseError(lua) : 1;
}

Expand All @@ -772,12 +772,6 @@ int RedisGenericCommand(lua_State *lua, int raise_error) {
return raise_error ? RaiseError(lua) : 1;
}

// TODO: fix blocking commands to make them work in scripting
if (cmd_flags & redis::kCmdBlocking) {
PushError(lua, "This Redis command is not allowed from scripts");
return raise_error ? RaiseError(lua) : 1;
}

std::string cmd_name = attributes->name;

auto *conn = script_run_ctx->conn;
Expand Down
8 changes: 7 additions & 1 deletion tests/gocase/unit/scripting/scripting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,16 @@ return {type(foo),foo == false}
})

t.Run("EVAL - Scripts can't run certain commands", func(t *testing.T) {
r := rdb.Eval(ctx, `return redis.pcall('blpop','x',0)`, []string{})
r := rdb.Eval(ctx, `return redis.pcall('shutdown')`, []string{})
require.ErrorContains(t, r.Err(), "not allowed")
})

t.Run("EVAL - Scripts can run blocking commands and get immediate result", func(t *testing.T) {
r := rdb.Eval(ctx, `return redis.pcall('blpop', KEYS[1], 0)`, []string{"key_for_blpop_script"})
require.Equal(t, r.Val(), nil)
require.ErrorContains(t, r.Err(), "nil")
})

t.Run("EVAL - Scripts can run certain commands", func(t *testing.T) {
r := rdb.Eval(ctx, `redis.pcall('randomkey'); return redis.pcall('set','x','ciao')`, []string{})
require.NoError(t, r.Err())
Expand Down
Loading