redis_server_protocol: make parsing string as member of redisCommandParser

src/brpc/policy/redis_protocol.cpp

@@ -79,7 +79,6 @@ public:
     bthread::ExecutionQueueId<std::string*> queue;
 
     RedisCommandParser parser;
-    std::string command;
 };
 
 int ConsumeTask(RedisConnContext* ctx, std::string* command, butil::IOBuf* sendbuf) {
@@ -195,12 +194,12 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket,
             }
             socket->reset_parsing_context(ctx);
         }
-        ParseError err = ctx->parser.ParseCommand(*source, &ctx->command);
+        ParseError err = ctx->parser.ParseCommand(*source);
         if (err != PARSE_OK) {
             return MakeParseError(err);
         }
         std::unique_ptr<std::string> command(new std::string);
-        command->swap(ctx->command);
+        command->swap(ctx->parser.Command());
         if (bthread::execution_queue_execute(ctx->queue, command.get()) != 0) {
             LOG(ERROR) << "Fail to push execution queue";
             return MakeParseError(PARSE_ERROR_NO_RESOURCE);

src/brpc/redis_command.cpp

@@ -364,7 +364,7 @@ RedisCommandParser::RedisCommandParser() {
     Reset();
 }
 
-ParseError RedisCommandParser::ParseCommand(butil::IOBuf& buf, std::string* out) {
+ParseError RedisCommandParser::ParseCommand(butil::IOBuf& buf) {
     const char* pfc = (const char*)buf.fetch1();
     if (pfc == NULL) {
         return PARSE_ERROR_NOT_ENOUGH_DATA;
@@ -395,13 +395,11 @@ ParseError RedisCommandParser::ParseCommand(butil::IOBuf& buf, std::string* out)
         _parsing_array = true;
         _length = value;
         _index = 0;
-        return ParseCommand(buf, out);
-    }
-    if (_index >= _length) {
-        LOG(WARNING) << "a complete command has been parsed. Do you forget "
-                "to call RedisCommandParser.Reset()?";
-        return PARSE_ERROR_ABSOLUTELY_WRONG;
+        _command.clear();
+        return ParseCommand(buf);
     }
+    CHECK(_index < _length) << "a complete command has been parsed. "
+            "impl of RedisCommandParser::ParseCommand is buggy";
     const int64_t len = value;  // `value' is length of the string
     if (len < 0) {
         LOG(ERROR) << "string in command is nil!";
@@ -416,10 +414,10 @@ ParseError RedisCommandParser::ParseCommand(butil::IOBuf& buf, std::string* out)
         return PARSE_ERROR_NOT_ENOUGH_DATA;
     }
     buf.pop_front(crlf_pos + 2/*CRLF*/);
-    if (!out->empty()) {
-        out->push_back(' ');    // command is separated by ' '
+    if (!_command.empty()) {
+        _command.push_back(' ');    // command is separated by ' '
     }
-    buf.cutn(out, len);
+    buf.cutn(&_command, len);
     char crlf[2];
     buf.cutn(crlf, sizeof(crlf));
     if (crlf[0] != '\r' || crlf[1] != '\n') {
@@ -427,7 +425,7 @@ ParseError RedisCommandParser::ParseCommand(butil::IOBuf& buf, std::string* out)
         return PARSE_ERROR_ABSOLUTELY_WRONG;
     }
     if (++_index < _length) {
-        return ParseCommand(buf, out);
+        return ParseCommand(buf);
     }
     Reset();
     return PARSE_OK;

src/brpc/redis_command.h

@@ -45,8 +45,12 @@ class RedisCommandParser {
 public:
     RedisCommandParser();
 
-    // Parse raw message from `buf' and write the result to `out'.
-    ParseError ParseCommand(butil::IOBuf& buf, std::string* out);
+    // Parse raw message from `buf'. Return PARSE_OK if successful.
+    ParseError ParseCommand(butil::IOBuf& buf);
+
+    // After ParseCommand returns PARSE_OK, call this function to get
+    // the parsed command string.
+    std::string& Command() { return _command; }
 
 private:
     // Reset parser to the initial state.
@@ -55,6 +59,7 @@ private:
     bool _parsing_array; // if the parser has met array indicator '*'
     int _length;         // array length
     int _index;          // current parsing array index
+    std::string _command; // parsed command string
 };
 
 } // namespace brpc

test/brpc_redis_unittest.cpp

@@ -557,64 +557,52 @@ TEST_F(RedisTest, command_parser) {
         // parse from whole command
         std::string command = "set abc edc";
         ASSERT_TRUE(brpc::RedisCommandNoFormat(&buf, command.c_str()).ok());
-        std::string command_out;
-        ASSERT_EQ(brpc::PARSE_OK, parser.ParseCommand(buf, &command_out));
+        ASSERT_EQ(brpc::PARSE_OK, parser.ParseCommand(buf));
         ASSERT_TRUE(buf.empty());
-        ASSERT_STREQ(command.c_str(), command_out.c_str());
+        ASSERT_STREQ(command.c_str(), parser.Command().c_str());
     }
     {
         // parse from two consecutive buf
         buf.append("*3\r\n$3");
-        std::string command_out;
         ASSERT_EQ(brpc::PARSE_ERROR_NOT_ENOUGH_DATA,
-                parser.ParseCommand(buf, &command_out));
+                parser.ParseCommand(buf));
         ASSERT_EQ((int)buf.size(), 2);    // left "$3"
         buf.append("\r\nset\r\n$3\r\nabc\r\n$3\r\ndef\r\n");
-        ASSERT_EQ(brpc::PARSE_OK, parser.ParseCommand(buf, &command_out));
+        ASSERT_EQ(brpc::PARSE_OK, parser.ParseCommand(buf));
         ASSERT_TRUE(buf.empty());
-        ASSERT_STREQ(command_out.c_str(), "set abc def");
+        ASSERT_STREQ(parser.Command().c_str(), "set abc def");
     }
     {
-        // there is a non-string message and parse should fail
+        // there is a non-string message in command and parse should fail
         buf.append("*3\r\n$3");
-        std::string command_out;
-        ASSERT_EQ(brpc::PARSE_ERROR_NOT_ENOUGH_DATA,
-                parser.ParseCommand(buf, &command_out));
+        ASSERT_EQ(brpc::PARSE_ERROR_NOT_ENOUGH_DATA, parser.ParseCommand(buf));
         ASSERT_EQ((int)buf.size(), 2);    // left "$3"
         buf.append("\r\nset\r\n:123\r\n$3\r\ndef\r\n");
-        ASSERT_EQ(brpc::PARSE_ERROR_ABSOLUTELY_WRONG, parser.ParseCommand(buf, &command_out));
+        ASSERT_EQ(brpc::PARSE_ERROR_ABSOLUTELY_WRONG, parser.ParseCommand(buf));
         parser.Reset();
     }
     {
         // not array
         buf.append(":123456\r\n");
-        std::string command_out;
-        ASSERT_EQ(brpc::PARSE_ERROR_TRY_OTHERS,
-                parser.ParseCommand(buf, &command_out));
+        ASSERT_EQ(brpc::PARSE_ERROR_TRY_OTHERS, parser.ParseCommand(buf));
         parser.Reset();
     }
     {
         // not array
         buf.append("+Error\r\n");
-        std::string command_out;
-        ASSERT_EQ(brpc::PARSE_ERROR_TRY_OTHERS,
-                parser.ParseCommand(buf, &command_out));
+        ASSERT_EQ(brpc::PARSE_ERROR_TRY_OTHERS, parser.ParseCommand(buf));
         parser.Reset();
     }
     {
         // not array
         buf.append("+OK\r\n");
-        std::string command_out;
-        ASSERT_EQ(brpc::PARSE_ERROR_TRY_OTHERS,
-                parser.ParseCommand(buf, &command_out));
+        ASSERT_EQ(brpc::PARSE_ERROR_TRY_OTHERS, parser.ParseCommand(buf));
         parser.Reset();
     }
     {
         // not array
         buf.append("$5\r\nhello\r\n");
-        std::string command_out;
-        ASSERT_EQ(brpc::PARSE_ERROR_TRY_OTHERS,
-                parser.ParseCommand(buf, &command_out));
+        ASSERT_EQ(brpc::PARSE_ERROR_TRY_OTHERS, parser.ParseCommand(buf));
         parser.Reset();
     }
 }