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.

Sign up for GitHub

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

fix: broken memcached error reporting #1741

Merged
merged 13 commits into from
Aug 28, 2023
Merged

fix: broken memcached error reporting #1741

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
eca852b
fix: broken memcached error reporting
kostasrim Aug 25, 2023
93118cc
Merge branch 'main' into fix_broken_memcached_error_reporting
kostasrim Aug 25, 2023
d12367d
chore: add test to reproduce improper error handling for memcache pro…
kostasrim Aug 25, 2023
7f185b7
fix: add missing import
kostasrim Aug 25, 2023
caaddd9
fix: address gh comments and fix bug in __init__.py
kostasrim Aug 26, 2023
cad0b95
Merge branch 'main' into fix_broken_memcached_error_reporting
kostasrim Aug 26, 2023
0bbd14b
feat: implement CONFIG GET command (#1751)
romange Aug 28, 2023
811a31d
Merge branch 'main' into fix_broken_memcached_error_reporting
kostasrim Aug 28, 2023
63c267e
chore: address gh comments
kostasrim Aug 28, 2023
ae6957d
fix broken build
kostasrim Aug 28, 2023
b47fb59
chore: address gh comments
kostasrim Aug 28, 2023
f601021
Merge branch 'main' into fix_broken_memcached_error_reporting
kostasrim Aug 28, 2023
0c96524
chore: remove temp file
kostasrim Aug 28, 2023
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
18 changes: 18 additions & 0 deletions src/facade/conn_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ class ConnectionContext {
}

// A convenient proxy for redis interface.
// Use with caution -- should only be used only
// in execution paths that are Redis *only*
RedisReplyBuilder* operator->();

SinkReplyBuilder* reply_builder() {
Expand All @@ -50,6 +52,22 @@ class ConnectionContext {
return res;
}

void SendError(std::string_view str, std::string_view type = std::string_view{}) {
rbuilder_->SendError(str, type);
}

void SendError(ErrorReply&& error) {
rbuilder_->SendError(std::move(error));
}

void SendError(OpStatus status) {
rbuilder_->SendError(status);
}

void SendSimpleString(std::string_view str) {
rbuilder_->SendSimpleString(str);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One day I'll make ErrorReply's constructor implicit and remove all the overloads 😆

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be nice.


// connection state / properties.
bool conn_closing : 1;
bool req_auth : 1;
Expand Down
18 changes: 15 additions & 3 deletions src/facade/reply_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,20 @@ void MCReplyBuilder::SendMGetResponse(absl::Span<const OptResp> arr) {
SendSimpleString("END");
}

void MCReplyBuilder::SendError(string_view str, std::string_view type) {
SendSimpleString("ERROR");
void MCReplyBuilder::SendError(string_view str, [[maybe_unused]] std::string_view type) {
SendClientError(str);
}

void MCReplyBuilder::SendError(ErrorReply error) {
if (error.status)
return SendError(*error.status);

string_view message_sv = visit([](auto&& str) -> string_view { return str; }, error.message);
SendError(message_sv, error.kind);
}

void MCReplyBuilder::SendError(OpStatus status) {
SendError(StatusToMsg(status));
}

void MCReplyBuilder::SendProtocolError(std::string_view str) {
Expand Down Expand Up @@ -277,7 +289,7 @@ void RedisReplyBuilder::SendBulkString(std::string_view str) {
return Send(v, ABSL_ARRAYSIZE(v));
}

std::string_view RedisReplyBuilder::StatusToMsg(OpStatus status) {
std::string_view SinkReplyBuilder::StatusToMsg(OpStatus status) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you change it? that's not good because these errors are not part of memcached protocol.

Copy link
Contributor Author

@kostasrim kostasrim Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not true:

   949     if (cid->IsTransactional()) {       
   950       dist_trans.reset(new Transaction{cid});                                                     
   951                
   952       if (!dist_trans->IsMulti()) {  // Multi command initialize themself based on their mode.
   953         if (auto st = dist_trans->InitByArgs(dfly_cntx->conn_state.db_index, args_no_cmd);        
   954             st != OpStatus::OK)                
   955           return cntx->SendError(st);
   956       }                                                                                           
   957                                                                    
   958       dfly_cntx->transaction = dist_trans.get();
   959       dfly_cntx->last_command_debug.shards_count = dfly_cntx->transaction->GetUniqueShardCnt();   
   960     } else {                                                                                      
   961       dfly_cntx->transaction = nullptr;                                                           
   962     }                                                                        
   963   }

Andddd

<< CI{"SET", CO::WRITE | CO::DENYOOM | CO::NO_AUTOJOURNAL

So memcached commands are transactional. Same applies for get incr decr etc. If the weren't, our transactional system would be broken, because memcached commands would not be a part of a transaction whereas redis commands would causing all sorts of broken behavior when interleaved.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitByArgs always succeeds for these commands.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We basically validate everything for memcached already at the parser level

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really bug prone tbh, some of the error reporting is already handled while some other doesn't(in case of this bug). Anyway, beyond the scope of this PR. I will see at some point how we can simplify this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this for now

Copy link
Contributor Author

@kostasrim kostasrim Aug 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually no, we kinda need this because we need SendError(ErrorReply) which uses status.

to be fair, it won't ever arrive to that block that uses status so I ended up removing this and added a dcheck for our sanity shake. You never know if we hit that so at least let's report this to be sure.

switch (status) {
case OpStatus::OK:
return "OK";
Expand Down
16 changes: 10 additions & 6 deletions src/facade/reply_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ class SinkReplyBuilder {
}

virtual void SendError(std::string_view str, std::string_view type = {}) = 0; // MC and Redis
virtual void SendError(ErrorReply error) = 0;
virtual void SendError(OpStatus status) = 0;

virtual void SendStored() = 0; // Reply for set commands.
virtual void SendSetSkipped() = 0;
Expand All @@ -57,6 +59,10 @@ class SinkReplyBuilder {

virtual void SendProtocolError(std::string_view str) = 0;

// You normally should not call this - maps the status
// into the string that would be sent
static std::string_view StatusToMsg(OpStatus status);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not your fault but those local fixes make the whole thing more confusing.
Let's do the following:

  1. Move StatusToMsg to op_status.h as std::string_view StastusMessage(OpStatus status);
  2. let's make SendError(ErrorReply error) non virtual method in SinkReplyBuilder.

I also changed how SendError is implemented for MC reply builder, I will send you a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make SendError(ErrorReply error) non virtual method in SinkReplyBuilder.

We can't really do that because of void CapturingReplyBuilder::SendError(ErrorReply error) actually overrides it with a different implementation.

Copy link
Contributor

@dranikpg dranikpg Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It overwrites the others as well (string_view), it just becomes a little less efficient because it can't move out of the string IIRC


// In order to reduce interrupt rate we allow coalescing responses together using
// Batch mode. It is controlled by Connection state machine because it makes sense only
// when pipelined requests are arriving.
Expand Down Expand Up @@ -148,6 +154,8 @@ class MCReplyBuilder : public SinkReplyBuilder {
using SinkReplyBuilder::SendRaw;

void SendError(std::string_view str, std::string_view type = std::string_view{}) final;
void SendError(ErrorReply error) final;
void SendError(OpStatus status) final;

// void SendGetReply(std::string_view key, uint32_t flags, std::string_view value) final;
void SendMGetResponse(absl::Span<const OptResp>) final;
Expand Down Expand Up @@ -177,13 +185,13 @@ class RedisReplyBuilder : public SinkReplyBuilder {
void SetResp3(bool is_resp3);

void SendError(std::string_view str, std::string_view type = {}) override;
virtual void SendError(ErrorReply error);
void SendError(ErrorReply error) override;

void SendMGetResponse(absl::Span<const OptResp>) override;

void SendStored() override;
void SendSetSkipped() override;
virtual void SendError(OpStatus status);
void SendError(OpStatus status) override;
void SendProtocolError(std::string_view str) override;

virtual void SendNullArray(); // Send *-1
Expand All @@ -206,10 +214,6 @@ class RedisReplyBuilder : public SinkReplyBuilder {

static char* FormatDouble(double val, char* dest, unsigned dest_len);

// You normally should not call this - maps the status
// into the string that would be sent
static std::string_view StatusToMsg(OpStatus status);

protected:
struct WrappedStrSpan : public StrSpan {
size_t Size() const;
Expand Down
14 changes: 7 additions & 7 deletions src/server/main_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,7 @@ void Service::DispatchCommand(CmdArgList args, facade::ConnectionContext* cntx)
const auto [cid, args_no_cmd] = FindCmd(args);

if (cid == nullptr) {
return (*cntx)->SendError(ReportUnknownCmd(ArgS(args, 0)));
return cntx->SendError(ReportUnknownCmd(ArgS(args, 0)));
}

ConnectionContext* dfly_cntx = static_cast<ConnectionContext*>(cntx);
Expand All @@ -905,7 +905,7 @@ void Service::DispatchCommand(CmdArgList args, facade::ConnectionContext* cntx)
if (auto& exec_info = dfly_cntx->conn_state.exec_info; exec_info.IsCollecting())
exec_info.state = ConnectionState::ExecInfo::EXEC_ERROR;

(*dfly_cntx)->SendError(std::move(*err));
dfly_cntx->SendError(std::move(*err));
return;
}

Expand All @@ -915,16 +915,16 @@ void Service::DispatchCommand(CmdArgList args, facade::ConnectionContext* cntx)
StoredCmd stored_cmd{cid, args_no_cmd};
dfly_cntx->conn_state.exec_info.body.push_back(std::move(stored_cmd));

return (*cntx)->SendSimpleString("QUEUED");
return cntx->SendSimpleString("QUEUED");
}

uint64_t start_ns = absl::GetCurrentTimeNanos();

if (cid->opt_mask() & CO::DENYOOM) {
int64_t used_memory = etl.GetUsedMemory(start_ns);
uint64_t used_memory = etl.GetUsedMemory(start_ns);
double oom_deny_ratio = GetFlag(FLAGS_oom_deny_ratio);
if (used_memory > (max_memory_limit * oom_deny_ratio)) {
return (*cntx)->SendError(kOutOfMemory);
return cntx->SendError(kOutOfMemory);
}
}

Expand All @@ -941,7 +941,7 @@ void Service::DispatchCommand(CmdArgList args, facade::ConnectionContext* cntx)
dfly_cntx->transaction->InitByArgs(dfly_cntx->conn_state.db_index, args_no_cmd);

if (status != OpStatus::OK)
return (*cntx)->SendError(status);
return cntx->SendError(status);
}
} else {
DCHECK(dfly_cntx->transaction == nullptr);
Expand All @@ -952,7 +952,7 @@ void Service::DispatchCommand(CmdArgList args, facade::ConnectionContext* cntx)
if (!dist_trans->IsMulti()) { // Multi command initialize themself based on their mode.
if (auto st = dist_trans->InitByArgs(dfly_cntx->conn_state.db_index, args_no_cmd);
st != OpStatus::OK)
return (*cntx)->SendError(st);
return cntx->SendError(st);
}

dfly_cntx->transaction = dist_trans.get();
Expand Down
6 changes: 3 additions & 3 deletions src/server/string_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -976,7 +976,7 @@ void StringFamily::IncrBy(CmdArgList args, ConnectionContext* cntx) {
int64_t val;

if (!absl::SimpleAtoi(sval, &val)) {
return (*cntx)->SendError(kInvalidIntErr);
return cntx->SendError(kInvalidIntErr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i know it looks fragile but memcache parser already does all the parsing checks because those conditions should not trigger with memcache.

}
return IncrByGeneric(key, val, cntx);
}
Expand Down Expand Up @@ -1016,10 +1016,10 @@ void StringFamily::DecrBy(CmdArgList args, ConnectionContext* cntx) {
int64_t val;

if (!absl::SimpleAtoi(sval, &val)) {
return (*cntx)->SendError(kInvalidIntErr);
return cntx->SendError(kInvalidIntErr);
}
if (val == INT64_MIN) {
return (*cntx)->SendError(kIncrOverflow);
return cntx->SendError(kIncrOverflow);
}

return IncrByGeneric(key, -val, cntx);
Expand Down