From 0facd6fd8f6d5d8d145bc799a21a0422f63d4e6c Mon Sep 17 00:00:00 2001 From: Kostas Kyrimis Date: Mon, 11 Nov 2024 19:53:30 +0200 Subject: [PATCH] fix: skip Send() in SinkReplyBuilder::Flush() when vec is empty (#4114) * skip Send() in SinkReplyBuilder::Flush() when vec is empty Signed-off-by: kostas --- src/facade/reply_builder.cc | 5 ++++- src/server/string_family.cc | 21 ++++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/facade/reply_builder.cc b/src/facade/reply_builder.cc index 3ca92b1c9..0e675901c 100644 --- a/src/facade/reply_builder.cc +++ b/src/facade/reply_builder.cc @@ -127,7 +127,8 @@ void SinkReplyBuilder::WriteRef(std::string_view str) { } void SinkReplyBuilder::Flush(size_t expected_buffer_cap) { - Send(); + if (!vecs_.empty()) + Send(); // Grow backing buffer if was at least half full and still below it's max size if (buffer_.InputLen() * 2 > buffer_.Capacity() && buffer_.Capacity() * 2 <= kMaxBufferSize) @@ -144,6 +145,8 @@ void SinkReplyBuilder::Flush(size_t expected_buffer_cap) { } void SinkReplyBuilder::Send() { + DCHECK(sink_ != nullptr); + DCHECK(!vecs_.empty()); auto& reply_stats = tl_facade_stats->reply_stats; send_active_ = true; diff --git a/src/server/string_family.cc b/src/server/string_family.cc index 22bbb65e1..0d9f23289 100644 --- a/src/server/string_family.cc +++ b/src/server/string_family.cc @@ -1313,9 +1313,28 @@ void StringFamily::MGet(CmdArgList args, Transaction* tx, SinkReplyBuilder* buil } } + // The code below is safe in the context of squashing (uses CapturingReplyBuilder). + // Specifically: + // 1. For Memcache: + // builder != CapturingReplyBuilder here because this is only used in squashing + // and there are only two cases: + // * Squashing the pipeline something that is turned off when using MEMCACHE. + // * Squashing a multi/exec block. There exist no such command in MEMCACHE. + // Therefore this path is safe, and the DCHECK in the if statement below shall + // never trigger. + // 2. For Redis: + // * Call to StartArray() is safe because it calls RedisReplyBuilder::StartCollection which + // calls CapturingReplyBuilder::StartCollection + // * Calls to SendBulkString() and SendNull() find and if builder is CapturingReplyBuilder + // then the right member gets called. + // + // Finally, the ReplyScope will trigger a Flush() on scope's end. What that means is, + // for CapturingReplyBuilder the internal vec is empty and therefore we should skip the call + // to Send because sink_ is nullptr and there is no payload to Send since it was captured. SinkReplyBuilder::ReplyScope scope(builder); if (builder->GetProtocol() == Protocol::MEMCACHE) { auto* rb = static_cast(builder); + DCHECK(dynamic_cast(builder) == nullptr); for (const auto& entry : res) { if (!entry) continue; @@ -1588,7 +1607,7 @@ void StringFamily::Register(CommandRegistry* registry) { << CI{"SUBSTR", CO::READONLY, 4, 1, 1}.HFUNC(GetRange) // Alias for GetRange << CI{"SETRANGE", CO::WRITE | CO::DENYOOM, 4, 1, 1}.HFUNC(SetRange) << CI{"CL.THROTTLE", CO::WRITE | CO::DENYOOM | CO::FAST, -5, 1, 1, acl::THROTTLE}.HFUNC( - ClThrottle); + ClThrottle); } } // namespace dfly