From 239240a0cca2867591c8efa36e7b81847c02d2da Mon Sep 17 00:00:00 2001 From: adi_holden Date: Wed, 18 Sep 2024 00:28:08 +0300 Subject: [PATCH 1/3] fix server: fix last error reply Signed-off-by: adi_holden --- src/facade/reply_capture.cc | 3 +++ src/server/main_service.cc | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/facade/reply_capture.cc b/src/facade/reply_capture.cc index 142b87e41de7..399a4f71ab82 100644 --- a/src/facade/reply_capture.cc +++ b/src/facade/reply_capture.cc @@ -29,6 +29,9 @@ void CapturingReplyBuilder::SendMGetResponse(MGetResponse resp) { } void CapturingReplyBuilder::SendError(OpStatus status) { + if (status != OpStatus::OK) { + last_error_ = StatusToMsg(status); + } SKIP_LESS(ReplyMode::ONLY_ERR); Capture(status); } diff --git a/src/server/main_service.cc b/src/server/main_service.cc index be0f002558c3..da071ec24155 100644 --- a/src/server/main_service.cc +++ b/src/server/main_service.cc @@ -1331,7 +1331,7 @@ OpResult OpTrackKeys(const OpArgs slice_args, const facade::Connection::We bool Service::InvokeCmd(const CommandId* cid, CmdArgList tail_args, ConnectionContext* cntx) { DCHECK(cid); DCHECK(!cid->Validate(tail_args)); - + DCHECK(cntx->reply_builder()->ConsumeLastError().empty()); if (auto err = VerifyCommandExecution(cid, cntx, tail_args); err) { // We need to skip this because ACK's should not be replied to // Bonus points because this allows to continue replication with ACL users who got From 81a07d242d1634ff72829b92a20808209e2beed1 Mon Sep 17 00:00:00 2001 From: adi_holden Date: Wed, 18 Sep 2024 00:35:46 +0300 Subject: [PATCH 2/3] place the dcheck in the right place Signed-off-by: adi_holden --- src/server/main_service.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/server/main_service.cc b/src/server/main_service.cc index da071ec24155..1067bd2596ab 100644 --- a/src/server/main_service.cc +++ b/src/server/main_service.cc @@ -1331,7 +1331,7 @@ OpResult OpTrackKeys(const OpArgs slice_args, const facade::Connection::We bool Service::InvokeCmd(const CommandId* cid, CmdArgList tail_args, ConnectionContext* cntx) { DCHECK(cid); DCHECK(!cid->Validate(tail_args)); - DCHECK(cntx->reply_builder()->ConsumeLastError().empty()); + if (auto err = VerifyCommandExecution(cid, cntx, tail_args); err) { // We need to skip this because ACK's should not be replied to // Bonus points because this allows to continue replication with ACL users who got @@ -1375,6 +1375,7 @@ bool Service::InvokeCmd(const CommandId* cid, CmdArgList tail_args, ConnectionCo ReplyGuard reply_guard(cntx, cid->name()); #endif uint64_t invoke_time_usec = 0; + DCHECK(cntx->reply_builder()->ConsumeLastError().empty()); try { invoke_time_usec = cid->Invoke(tail_args, cntx); } catch (std::exception& e) { From 0e3451c691edc2d8507cc73fdfc146c5b9bc85cc Mon Sep 17 00:00:00 2001 From: kostas Date: Thu, 19 Sep 2024 16:17:43 +0300 Subject: [PATCH 3/3] fixes --- src/facade/reply_capture.cc | 2 ++ src/server/main_service.cc | 5 ++++- src/server/multi_command_squasher.cc | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/facade/reply_capture.cc b/src/facade/reply_capture.cc index 399a4f71ab82..c799e6a818a3 100644 --- a/src/facade/reply_capture.cc +++ b/src/facade/reply_capture.cc @@ -218,6 +218,8 @@ void CapturingReplyBuilder::Apply(Payload&& pl, RedisReplyBuilder* rb) { CaptureVisitor cv{rb}; visit(cv, std::move(pl)); + // Consumed and printed by InvokeCmd. We just send the actual error here + std::ignore = rb->ConsumeLastError(); } void CapturingReplyBuilder::SetReplyMode(ReplyMode mode) { diff --git a/src/server/main_service.cc b/src/server/main_service.cc index 1067bd2596ab..133edba6188a 100644 --- a/src/server/main_service.cc +++ b/src/server/main_service.cc @@ -1172,6 +1172,8 @@ std::optional Service::VerifyCommandState(const CommandId* cid, CmdA } void Service::DispatchCommand(CmdArgList args, facade::ConnectionContext* cntx) { + absl::Cleanup clear_last_error( + [cntx]() { std::ignore = cntx->reply_builder()->ConsumeLastError(); }); CHECK(!args.empty()); DCHECK_NE(0u, shard_set->size()) << "Init was not called"; @@ -1375,7 +1377,8 @@ bool Service::InvokeCmd(const CommandId* cid, CmdArgList tail_args, ConnectionCo ReplyGuard reply_guard(cntx, cid->name()); #endif uint64_t invoke_time_usec = 0; - DCHECK(cntx->reply_builder()->ConsumeLastError().empty()); + auto last_error = cntx->reply_builder()->ConsumeLastError(); + DCHECK(last_error.empty()); try { invoke_time_usec = cid->Invoke(tail_args, cntx); } catch (std::exception& e) { diff --git a/src/server/multi_command_squasher.cc b/src/server/multi_command_squasher.cc index 5872252d3092..cfa52e5d1667 100644 --- a/src/server/multi_command_squasher.cc +++ b/src/server/multi_command_squasher.cc @@ -121,6 +121,7 @@ bool MultiCommandSquasher::ExecuteStandalone(StoredCmd* cmd) { if (verify_commands_) { if (auto err = service_->VerifyCommandState(cmd->Cid(), args, *cntx_); err) { cntx_->SendError(std::move(*err)); + std::ignore = cntx_->reply_builder()->ConsumeLastError(); return !error_abort_; } }