Copilot commented on code in PR #3505:
URL: https://github.com/apache/kvrocks/pull/3505#discussion_r3315890639
##########
src/server/redis_connection.cc:
##########
@@ -433,6 +454,18 @@ void Connection::ExecuteCommands(std::deque<CommandTokens>
*to_process_cmds) {
to_process_cmds->pop_front();
if (cmd_tokens.empty()) continue;
+ bool is_script_kill = (util::EqualICase(cmd_tokens.front(), "script") &&
cmd_tokens.size() >= 2 &&
+ util::EqualICase(cmd_tokens[1], "kill"));
+ bool is_shutdown = util::EqualICase(cmd_tokens.front(), "shutdown");
+
+ if (srv_->IsScriptTimedOut()) {
+ if (!is_script_kill && !is_shutdown) {
+ Reply(redis::Error({Status::RedisErrorNoPrefix,
+ "BUSY Redis is busy running a script. You can only
call SCRIPT KILL or SHUTDOWN NOSAVE."}));
+ continue;
+ }
+ }
Review Comment:
The fast-path detection for `SCRIPT KILL` only checks `cmd_tokens.size() >=
2` and `cmd_tokens[1] == "kill"`, so an oversized command like `SCRIPT KILL
extra args` (which is invalid arity) will incorrectly bypass the BUSY check
here. Also, since BUSY interception happens before authentication/namespace
checks, any unauthenticated client can issue `SCRIPT KILL` / `SHUTDOWN` tokens
to bypass the BUSY reply path (the command itself will still fail on auth
later, but the BUSY semantics are skipped). Consider tightening the matcher to
exactly two tokens for `SCRIPT KILL` and to verify auth/admin requirements
before bypassing.
##########
src/server/worker.cc:
##########
@@ -582,6 +598,36 @@ void Worker::LuaReset() {
int64_t Worker::GetLuaMemorySize() { return (int64_t)lua_gc(lua_, LUA_GCCOUNT,
0) * 1024; }
+void Worker::PollEventLoop() {
+ event_base_loop(base_, EVLOOP_ONCE | EVLOOP_NONBLOCK);
+
+ // Flush all active connections' output buffers to their sockets
+ std::lock_guard<std::mutex> lock(conns_mu_);
+ for (const auto &iter : conns_) {
+ auto *conn = iter.second;
+ if (conn->IsFlagEnabled(redis::Connection::kCloseAsync)) {
+ continue;
+ }
+ auto *bev = conn->GetBufferEvent();
+ if (bev) {
+ bool is_tls = false;
+#ifdef ENABLE_OPENSSL
+ if (bufferevent_openssl_get_ssl(bev) != nullptr) {
+ is_tls = true;
+ }
+#endif
+ if (is_tls) {
+ bufferevent_flush(bev, EV_WRITE, BEV_FLUSH);
+ } else {
+ auto *output = bufferevent_get_output(bev);
+ if (evbuffer_get_length(output) > 0) {
+ evbuffer_write(output, conn->GetFD());
+ }
Review Comment:
`evbuffer_write` returns an `ssize_t` and can fail (e.g., `EAGAIN`, `EPIPE`)
or perform a partial write; the return value is ignored here. On a partial
write, the remaining bytes will stay in the output buffer until the worker's
normal event loop processes them (which is the expected fallback), but a hard
error such as a broken pipe will be silently swallowed and the connection state
may diverge from libevent's expectations. Consider at least logging
non-recoverable errors. Additionally, calling `evbuffer_write` directly on a
bufferevent's output buffer bypasses libevent's bufferevent write machinery
(watermarks, write callbacks, rate limiting) — confirm this is acceptable for
all non-TLS bufferevents used here.
##########
src/server/server.cc:
##########
@@ -1879,6 +1894,64 @@ StatusOr<std::unique_ptr<redis::Commander>>
Server::LookupAndCreateCommand(const
return std::move(cmd);
}
+void Server::RegisterRunningScript(lua::ScriptRunCtx *rctx) {
+ std::lock_guard<std::mutex> guard(running_scripts_mu_);
+ running_scripts_.push_back(rctx);
+}
+
+void Server::UnregisterRunningScript(lua::ScriptRunCtx *rctx) {
+ std::lock_guard<std::mutex> guard(running_scripts_mu_);
+ auto it = std::find(running_scripts_.begin(), running_scripts_.end(), rctx);
+ if (it != running_scripts_.end()) {
+ running_scripts_.erase(it);
+ }
+
+ // Re-evaluate if any remaining scripts are timed out, if not, clear the flag
+ bool any_timed_out = false;
+ int limit = config_->lua_time_limit;
+ if (limit > 0 && !running_scripts_.empty()) {
+ uint64_t now_ms = util::GetTimeStampMS();
+ for (const auto *ctx : running_scripts_) {
+ if (now_ms - ctx->start_time_ms >= static_cast<uint64_t>(limit)) {
+ any_timed_out = true;
+ break;
+ }
+ }
+ }
+ is_script_timeout_.store(any_timed_out, std::memory_order_relaxed);
+}
+
+bool Server::IsScriptTimedOut() const { return
is_script_timeout_.load(std::memory_order_relaxed); }
+
Review Comment:
`SetScriptTimedOut(true)` is called from the Lua hook whenever any running
script exceeds `lua_time_limit`, but the flag is only re-evaluated and
potentially cleared inside `UnregisterRunningScript`. If `lua_time_limit` is
increased via `CONFIG SET` while a script is running, or the limit is set to 0
to disable it, `is_script_timeout_` will remain `true` until the offending
script finishes — meaning the server stays in BUSY mode rejecting all commands
even though the operator just disabled the timeout. Consider also re-evaluating
the flag when the config changes, or evaluating timeout status from the running
scripts list lazily rather than caching it.
##########
src/storage/scripting.cc:
##########
@@ -60,6 +63,100 @@ enum {
namespace lua {
+class ScriptRunCtxGuard {
+ public:
+ ScriptRunCtxGuard(Server *srv, lua_State *lua, ScriptRunCtx *rctx) :
srv_(srv), lua_(lua), rctx_(rctx) {
+ rctx_->start_time_ms = util::GetTimeStampMS();
+ SaveOnRegistry(lua_, REGISTRY_SCRIPT_RUN_CTX_NAME, rctx_);
+ srv_->RegisterRunningScript(rctx_);
+ lua_sethook(lua_, LuaMaskCountHook, LUA_MASKCOUNT, 100000);
+ }
+
+ ~ScriptRunCtxGuard() {
+ lua_sethook(lua_, nullptr, 0, 0);
+ srv_->UnregisterRunningScript(rctx_);
+ RemoveFromRegistry(lua_, REGISTRY_SCRIPT_RUN_CTX_NAME);
+ }
+
+ private:
+ Server *srv_;
+ lua_State *lua_;
+ ScriptRunCtx *rctx_;
+};
+
+static void KillScript(lua_State *lua) {
+ lua_sethook(lua, LuaMaskCountHook, LUA_MASKLINE, 0);
Review Comment:
`KillScript` sets the hook mask to `LUA_MASKLINE` with a count of `0` before
raising the Lua error. Per Lua semantics, `lua_sethook` with `mask == 0` (or
count == 0 when only MASKCOUNT is in the mask) disables the hook; using
`LUA_MASKLINE` here is unusual and the comment/intent should be clarified. If
the intent is to ensure the error propagates and the hook is no longer
re-entered, consider explicitly disabling the hook with `lua_sethook(lua,
nullptr, 0, 0)` instead, which matches the cleanup in `ScriptRunCtxGuard`'s
destructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
