fix: broken memcached error reporting #1741

kostasrim · 2023-08-25T09:18:49Z

Partially solves #1730 (because I suspect there is some other issue as well -- either the user it doing something illegal with the replica or there is some other error which is not reported properly because of this bug).

ConnectionContext overloads operator-> by returning a RedisReplyBuilder. This is extremely error prone, because there are execution paths that work both with memcached and redis protocol. If used improperly in a memcached execution path it will trigger an internal assertion Protocol::REDIS == protocol() instead of properly report the error to the client.

This PR addresses this, by adding the SendError family of functions in ConnectionContext. I replace the (*cntx)-> with cntx->SendError which uses the internal SinkReplyBuilder and dispatching to the right function based on the underline protocol.

All in all this PR fixes:

DispatchCommand error reporting (one example is when we use SET command on the replica -- previously we crashed now we properly report an error)
INCRBY command error reporting (this would crash)
DECRBY command error reporting (this would crash)

Basically, I went over all of the memcached commands just to make sure that we report errors properly via the common base client SinkReplyBuilder and not via the specific RedisReplyBuilder class via the overloaded arrow operator.

I really don't like the operator-> as it's extremely error prone and it's really hard to assert that an execution path is redis only (especially when both -> and reply_builder are used within the same function -- it makes you scratch your head if it's a bug or an actual feature).

Generally speaking a long term solution is to replace all of the common functions to strictly use a common interface via SinkReplyBuilder such that are protocol agnostic (since anyway they should be used by both memcached and redis)

romange · 2023-08-25T10:36:15Z

src/facade/reply_builder.cc

@@ -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) {


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

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.

InitByArgs always succeeds for these commands.

We basically validate everything for memcached already at the parser level

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.

I removed this for now

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.

romange · 2023-08-25T10:47:10Z

I think we should add a pytest that reproduces the problem.

dranikpg · 2023-08-25T11:10:24Z

src/facade/conn_context.h

+  void SendError(OpStatus status) {
+    rbuilder_->SendError(status);
+  }
+
+  void SendSimpleString(std::string_view str) {
+    rbuilder_->SendSimpleString(str);
+  }


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

it could be nice.

…tocol

romange · 2023-08-25T18:31:26Z

@kostasrim I added this test to the main branch (locally on my machine) and nothing has crashed.

kostasrim · 2023-08-26T11:56:06Z

@romange Yes because I forgot to check in the changes in __init__. We have another bug there:

105     @property
106     def mc_port(self) -> int:
107         if self.params.existi get(key: _KT) -> Optional[_VT_co]                              
108             return self.param get(key: _KT, default: Union[_VT_co, _T]) -> Union[_VT_co, _T] 
109         return int(self.args.get("mc_port", "11211"))

It works because you are not connecting to the replica. Will push in 5 and you should now get a nice error in main:

pymemcache.exceptions.MemcacheUnexpectedCloseError which is basically the replica crashing....

kostasrim · 2023-08-26T12:21:24Z

tests/dragonfly/__init__.py

@@ -106,7 +106,7 @@ def admin_port(self) -> int:
    def mc_port(self) -> int:
        if self.params.existing_mc_port:
            return self.params.existing_mc_port
-        return int(self.args.get("mc_port", "11211"))
+        return int(self.args.get("memcached_port", "11211"))


@romange this is why it was passing....

romange · 2023-08-26T13:38:50Z

src/server/string_family.cc

@@ -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);


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

romange · 2023-08-26T13:59:59Z

src/facade/reply_builder.h

@@ -57,6 +58,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);


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

Move StatusToMsg to op_status.h as std::string_view StastusMessage(OpStatus status);

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.

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.

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

* feat: implement CONFIG GET command The command returns all the matched arguments and their current values. In addition, this PR adds mutability semantics to each config - whether it can be changed at runtime. Fixes #1700 Signed-off-by: Roman Gershman <[email protected]> --------- Signed-off-by: Roman Gershman <[email protected]>

src/facade/reply_builder.cc

kostasrim · 2023-08-28T17:10:24Z

src/facade/reply_builder_test.cc

@@ -251,14 +251,39 @@ TEST_F(RedisReplyBuilderTest, ErrorBuiltInMessage) {
  }
 }

+TEST_F(RedisReplyBuilderTest, ErrorReplyBuiltInMessage) {


@romange This is a little "raw" but I wanted to verify that both of the execution paths are covered with ErrorReply without the extra nuances of printing extra error information through the << operator. Moreover, it doesn't cover all sorts possible OpStatus values and the e1, e2 are just placeholders such that I can verify that the right execution path gets triggered

kostasrim added 2 commits August 25, 2023 12:03

fix: broken memcached error reporting

eca852b

Merge branch 'main' into fix_broken_memcached_error_reporting

93118cc

kostasrim requested a review from romange August 25, 2023 09:18

kostasrim self-assigned this Aug 25, 2023

kostasrim added the bug Something isn't working label Aug 25, 2023

kostasrim enabled auto-merge (squash) August 25, 2023 09:24

romange reviewed Aug 25, 2023

View reviewed changes

dranikpg reviewed Aug 25, 2023

View reviewed changes

kostasrim added 2 commits August 25, 2023 15:22

chore: add test to reproduce improper error handling for memcache pro…

d12367d

…tocol

fix: add missing import

7f185b7

kostasrim requested a review from romange August 25, 2023 12:24

kostasrim added 2 commits August 26, 2023 15:17

fix: address gh comments and fix bug in __init__.py

caaddd9

Merge branch 'main' into fix_broken_memcached_error_reporting

cad0b95

kostasrim commented Aug 26, 2023

View reviewed changes

romange reviewed Aug 26, 2023

View reviewed changes

romange and others added 3 commits August 28, 2023 08:18

Merge branch 'main' into fix_broken_memcached_error_reporting

811a31d

chore: address gh comments

63c267e

kostasrim requested a review from romange August 28, 2023 10:44

kostasrim mentioned this pull request Aug 28, 2023

chore: remove unused op_status source and polish error reporting for #1752

Closed

fix broken build

ae6957d

romange reviewed Aug 28, 2023

View reviewed changes

src/facade/reply_builder.cc Show resolved Hide resolved

kostasrim disabled auto-merge August 28, 2023 12:54

romange force-pushed the main branch from 33e1e8c to 35a5433 Compare August 28, 2023 16:26

chore: address gh comments

b47fb59

kostasrim commented Aug 28, 2023

View reviewed changes

kostasrim added 2 commits August 28, 2023 20:11

Merge branch 'main' into fix_broken_memcached_error_reporting

f601021

chore: remove temp file

0c96524

kostasrim requested a review from romange August 28, 2023 17:14

kostasrim enabled auto-merge (squash) August 28, 2023 17:17

romange approved these changes Aug 28, 2023

View reviewed changes

kostasrim merged commit 1855c1c into main Aug 28, 2023

kostasrim deleted the fix_broken_memcached_error_reporting branch August 28, 2023 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: broken memcached error reporting #1741

fix: broken memcached error reporting #1741

kostasrim commented Aug 25, 2023

romange Aug 25, 2023

kostasrim Aug 25, 2023 •

edited

Loading

romange Aug 25, 2023

romange Aug 25, 2023

kostasrim Aug 26, 2023

kostasrim Aug 26, 2023

kostasrim Aug 26, 2023 •

edited

Loading

romange commented Aug 25, 2023

dranikpg Aug 25, 2023

romange Aug 26, 2023

romange commented Aug 25, 2023

kostasrim commented Aug 26, 2023

kostasrim Aug 26, 2023

romange Aug 26, 2023

romange Aug 26, 2023

kostasrim Aug 28, 2023

dranikpg Aug 28, 2023 •

edited

Loading

kostasrim Aug 28, 2023 •

edited

Loading

fix: broken memcached error reporting #1741

fix: broken memcached error reporting #1741

Conversation

kostasrim commented Aug 25, 2023

Choose a reason for hiding this comment

kostasrim Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Aug 26, 2023 • edited Loading

Choose a reason for hiding this comment

romange commented Aug 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange commented Aug 25, 2023

kostasrim commented Aug 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

kostasrim Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

kostasrim Aug 25, 2023 •

edited

Loading

kostasrim Aug 26, 2023 •

edited

Loading

dranikpg Aug 28, 2023 •

edited

Loading

kostasrim Aug 28, 2023 •

edited

Loading