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(conn): avoid to check the command name directly #2668

Open
wants to merge 6 commits into
base: unstable
Choose a base branch
from
Open
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/cmd_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1364,7 +1364,7 @@ REDIS_REGISTER_COMMANDS(Server, MakeCmdAttr<CommandAuth>("auth", 2, "read-only o
MakeCmdAttr<CommandSlaveOf>("slaveof", 3, "read-only exclusive no-script", NO_KEY),
MakeCmdAttr<CommandStats>("stats", 1, "read-only", NO_KEY),
MakeCmdAttr<CommandRdb>("rdb", -3, "write exclusive", NO_KEY),
MakeCmdAttr<CommandReset>("reset", 1, "ok-loading multi no-script", NO_KEY),
MakeCmdAttr<CommandReset>("reset", 1, "ok-loading bypass-multi no-script", NO_KEY),
MakeCmdAttr<CommandApplyBatch>("applybatch", -2, "write no-multi", NO_KEY),
MakeCmdAttr<CommandDump>("dump", 2, "read-only", 1, 1, 1),
MakeCmdAttr<CommandPollUpdates>("pollupdates", -2, "read-only", NO_KEY), )
Expand Down
14 changes: 5 additions & 9 deletions src/commands/cmd_txn.cc
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,6 @@ class CommandExec : public Commander {
class CommandWatch : public Commander {
public:
Status Execute([[maybe_unused]] engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override {
if (conn->IsFlagEnabled(Connection::kMultiExec)) {
return {Status::RedisExecErr, "WATCH inside MULTI is not allowed"};
}

// If a conn is already marked as watched_keys_modified, we can skip the watch.
if (srv->IsWatchedKeysModified(conn)) {
*output = redis::RESP_OK;
Expand All @@ -123,10 +119,10 @@ class CommandUnwatch : public Commander {
}
};

REDIS_REGISTER_COMMANDS(Txn, MakeCmdAttr<CommandMulti>("multi", 1, "multi", NO_KEY),
MakeCmdAttr<CommandDiscard>("discard", 1, "multi", NO_KEY),
MakeCmdAttr<CommandExec>("exec", 1, "exclusive multi slow", NO_KEY),
MakeCmdAttr<CommandWatch>("watch", -2, "multi", 1, -1, 1),
MakeCmdAttr<CommandUnwatch>("unwatch", 1, "multi", NO_KEY), )
REDIS_REGISTER_COMMANDS(Txn, MakeCmdAttr<CommandMulti>("multi", 1, "bypass-multi", NO_KEY),
MakeCmdAttr<CommandDiscard>("discard", 1, "bypass-multi", NO_KEY),
MakeCmdAttr<CommandExec>("exec", 1, "exclusive bypass-multi slow", NO_KEY),
MakeCmdAttr<CommandWatch>("watch", -2, "no-multi", 1, -1, 1),
MakeCmdAttr<CommandUnwatch>("unwatch", 1, "no-multi", NO_KEY), )

} // namespace redis
9 changes: 5 additions & 4 deletions src/commands/commander.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,9 @@ enum CommandFlags : uint64_t {
// "ok-loading" flag, for any command that can be executed while
// the db is in loading phase
kCmdLoading = 1ULL << 5,
// "multi" flag, for commands that can end a MULTI scope
kCmdEndMulti = 1ULL << 6,
// "bypass-multi" flag, for commands that can be executed in a MULTI scope,
// but these commands will NOT be queued and will be executed immediately
kCmdBypassMulti = 1ULL << 6,
// "exclusive" flag, for commands that should be executed execlusive globally
kCmdExclusive = 1ULL << 7,
// "no-multi" flag, for commands that cannot be executed in MULTI scope
Expand Down Expand Up @@ -320,8 +321,8 @@ inline uint64_t ParseCommandFlags(const std::string &description, const std::str
flags |= kCmdLoading;
else if (flag == "exclusive")
flags |= kCmdExclusive;
else if (flag == "multi")
flags |= kCmdEndMulti;
else if (flag == "bypass-multi")
flags |= kCmdBypassMulti;
else if (flag == "no-multi")
flags |= kCmdNoMulti;
else if (flag == "no-script")
Expand Down
5 changes: 5 additions & 0 deletions src/common/string_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ std::string ToLower(std::string in) {
return in;
}

std::string ToUpper(std::string in) {
std::transform(in.begin(), in.end(), in.begin(), [](char c) -> char { return static_cast<char>(std::toupper(c)); });
return in;
}

bool EqualICase(std::string_view lhs, std::string_view rhs) {
return lhs.size() == rhs.size() && std::equal(lhs.begin(), lhs.end(), rhs.begin(),
[](char l, char r) { return std::tolower(l) == std::tolower(r); });
Expand Down
1 change: 1 addition & 0 deletions src/common/string_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ namespace util {

std::string Float2String(double d);
std::string ToLower(std::string in);
std::string ToUpper(std::string in);
bool EqualICase(std::string_view lhs, std::string_view rhs);
std::string BytesToHuman(uint64_t n);
std::string Trim(std::string in, std::string_view chars);
Expand Down
6 changes: 3 additions & 3 deletions src/server/redis_connection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ void Connection::ExecuteCommands(std::deque<CommandTokens> *to_process_cmds) {
// that can guarantee other threads can't come into critical zone, such as DEBUG,
// CLUSTER subcommand, CONFIG SET, MULTI, LUA (in the immediate future).
// Otherwise, we just use 'ConcurrencyGuard' to allow all workers to execute commands at the same time.
if (is_multi_exec && cmd_name != "exec") {
if (is_multi_exec && !(cmd_flags & kCmdBypassMulti)) {
// No lock guard, because 'exec' command has acquired 'WorkExclusivityGuard'
} else if (cmd_flags & kCmdExclusive) {
exclusivity = srv_->WorkExclusivityGuard();
Expand Down Expand Up @@ -443,7 +443,7 @@ void Connection::ExecuteCommands(std::deque<CommandTokens> *to_process_cmds) {
}

if (is_multi_exec && (cmd_flags & kCmdNoMulti)) {
Reply(redis::Error({Status::NotOK, "Can't execute " + cmd_name + " in MULTI"}));
Reply(redis::Error({Status::NotOK, fmt::format("{} inside MULTI is not allowed", util::ToUpper(cmd_name))}));
multi_error_ = true;
continue;
}
Expand All @@ -463,7 +463,7 @@ void Connection::ExecuteCommands(std::deque<CommandTokens> *to_process_cmds) {
}

// We don't execute commands, but queue them, and then execute in EXEC command
if (is_multi_exec && !in_exec_ && !(cmd_flags & kCmdEndMulti)) {
if (is_multi_exec && !in_exec_ && !(cmd_flags & kCmdBypassMulti)) {
multi_cmds_.emplace_back(std::move(cmd_tokens));
Reply(redis::SimpleString("QUEUED"));
continue;
Expand Down
2 changes: 1 addition & 1 deletion tests/gocase/unit/multi/multi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func TestMulti(t *testing.T) {
t.Run("WATCH inside MULTI is not allowed", func(t *testing.T) {
require.NoError(t, rdb.Do(ctx, "MULTI").Err())
require.EqualError(t, rdb.Do(ctx, "WATCH", "x").Err(), "ERR WATCH inside MULTI is not allowed")
require.NoError(t, rdb.Do(ctx, "EXEC").Err())
require.NoError(t, rdb.Do(ctx, "DISCARD").Err())
})

t.Run("EXEC without MULTI is not allowed", func(t *testing.T) {
Expand Down
Loading