Skip to content

Commit a292dbc

Browse files
rajvarun77claude
andcommitted
test(mysql): clean-room integration tests + prepared-stmt error fix + Controller cleanup
- Add clean-room integration tests (transactions, prepared statements, pooled connection concurrency, connection-type) run against a self-spawned mysqld. - Fix: a failed COM_STMT_PREPARE now returns the ERR packet to the caller and keeps the connection alive, instead of closing the socket. - Warn when a prepared statement runs on a 'short' connection (re-prepares on every execute; prefer 'pooled'). - Replace Controller's mysql-specific _mysql_stmt with a generic opaque per-RPC slot so no protocol type leaks onto the shared Controller. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 91c9df8 commit a292dbc

9 files changed

Lines changed: 3007 additions & 22 deletions

src/brpc/controller.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -298,7 +298,7 @@ void Controller::ResetPods() {
298298
_response_streams.clear();
299299
_remote_stream_settings = NULL;
300300
_bind_sock_action = BIND_SOCK_NONE;
301-
_mysql_stmt = NULL;
301+
_session_data = NULL;
302302
_auth_flags = 0;
303303
_rpc_received_us = 0;
304304
}
@@ -829,7 +829,8 @@ void Controller::Call::OnComplete(
829829
if (sending_sock != NULL && (error_code == 0 || responded)) {
830830
if (bind_sock_action == BIND_SOCK_RESERVE) {
831831
// Reserve this socket on the controller for a following RPC
832-
// (mysql transaction / prepared statement connection affinity).
832+
// (mysql transaction connection affinity; prepared statements
833+
// do NOT reserve -- they use a per-socket stmt_id map + re-prepare).
833834
c->_bind_sock.reset(sending_sock.release());
834835
} else if (bind_sock_action == BIND_SOCK_USE) {
835836
// Socket is owned by the binder; do not return it to the pool.
@@ -1110,7 +1111,7 @@ void Controller::IssueRPC(int64_t start_realtime_us) {
11101111
SocketUniquePtr tmp_sock;
11111112
if ((_connection_type & CONNECTION_TYPE_POOLED_AND_SHORT) &&
11121113
_bind_sock_action == BIND_SOCK_USE) {
1113-
// Reuse the socket reserved by a previous RPC (mysql tx/stmt affinity).
1114+
// Reuse the socket reserved by a previous RPC (mysql transaction affinity).
11141115
tmp_sock.reset(_bind_sock.release());
11151116
if (!tmp_sock || (!is_health_check_call() && !tmp_sock->IsAvailable())) {
11161117
// NOTE: tmp_sock may be NULL here, so guard the id() deref.

src/brpc/controller.h

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -108,15 +108,14 @@ enum StopStyle {
108108
const int32_t UNSET_MAGIC_NUM = -123456789;
109109

110110
// If a controller wants to reserve the sending socket after the RPC (e.g. mysql
111-
// transactions/prepared statements that need connection affinity), set
112-
// BIND_SOCK_RESERVE; later RPCs reuse it via BIND_SOCK_USE.
111+
// transactions that need connection affinity), set BIND_SOCK_RESERVE; later RPCs
112+
// reuse it via BIND_SOCK_USE. (Prepared statements do NOT reserve -- they use a
113+
// per-socket stmt_id map + re-prepare instead of pinning a connection.)
113114
enum BindSockAction {
114115
BIND_SOCK_RESERVE,
115116
BIND_SOCK_USE,
116117
BIND_SOCK_NONE,
117118
};
118-
// mysql prepared statement, defined in mysql.h
119-
class MysqlStatementStub;
120119

121120
typedef butil::FlatMap<std::string, std::string> UserFieldsMap;
122121

@@ -927,13 +926,15 @@ friend void policy::ProcessThriftRequest(InputMessageBase*);
927926
// Defined at both sides
928927
StreamSettings *_remote_stream_settings;
929928

930-
// Whether/how to reserve the sending socket after the RPC (mysql tx/stmt).
929+
// Whether/how to reserve the sending socket after the RPC (mysql transactions).
931930
BindSockAction _bind_sock_action;
932931
// The socket reserved by a previous RPC and reused when _bind_sock_action
933932
// is BIND_SOCK_USE.
934933
SocketUniquePtr _bind_sock;
935-
// mysql prepared statement bound to this RPC, owned elsewhere.
936-
MysqlStatementStub* _mysql_stmt;
934+
// Opaque per-RPC slot a protocol codec may use to carry typed state from
935+
// serialize_request to pack_request/parse (e.g. the mysql prepared-statement
936+
// stub). Not owned by Controller.
937+
void* _session_data;
937938

938939
// Thrift method name, only used when thrift protocol enabled
939940
std::string _thrift_method_name;

src/brpc/details/controller_private_accessor.h

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ class ControllerPrivateAccessor {
134134

135135
void clear_auth_flags() { _cntl->_auth_flags = 0; }
136136

137-
// Set how the sending socket is reserved after the RPC (mysql tx/stmt).
137+
// Set how the sending socket is reserved after the RPC (mysql transactions).
138138
void set_bind_sock_action(BindSockAction action) { _cntl->_bind_sock_action = action; }
139139
// Transfer ownership of the reserved socket to `ptr`.
140140
void get_bind_sock(SocketUniquePtr* ptr) {
@@ -147,10 +147,8 @@ class ControllerPrivateAccessor {
147147
_cntl->_bind_sock_action = BIND_SOCK_USE;
148148
Socket::Address(sock_id, &_cntl->_bind_sock);
149149
}
150-
// Set the mysql prepared statement bound to this RPC.
151-
void set_mysql_stmt(MysqlStatementStub* stmt) { _cntl->_mysql_stmt = stmt; }
152-
// Get the mysql prepared statement bound to this RPC.
153-
MysqlStatementStub* mysql_stmt() { return _cntl->_mysql_stmt; }
150+
void set_session_data(void* d) { _cntl->_session_data = d; }
151+
void* session_data() const { return _cntl->_session_data; }
154152

155153
std::string& protocol_param() { return _cntl->protocol_param(); }
156154
const std::string& protocol_param() const { return _cntl->protocol_param(); }

src/brpc/mysql_statement.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,12 @@ void MysqlStatement::Init(const Channel& channel) {
6969
_connection_type = ConnectionType(opts.connection_type);
7070
if (_connection_type != CONNECTION_TYPE_SHORT) {
7171
_id_map.Modify(my_init_kv);
72+
} else {
73+
LOG_EVERY_SECOND(WARNING)
74+
<< "Prepared statement on a 'short' connection re-prepares on every "
75+
"execute (a new TCP connection per request cannot cache the "
76+
"server stmt_id); use connection_type='pooled' for prepared "
77+
"statements.";
7278
}
7379
}
7480

src/brpc/policy/mysql_protocol.cpp

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ bool PackRequest(butil::IOBuf* buf,
107107
LOG(ERROR) << "[MYSQL PACK] get sending socket with NULL";
108108
return false;
109109
}
110-
auto stub = accessor.mysql_stmt();
110+
auto stub = static_cast<MysqlStatementStub*>(accessor.session_data());
111111
if (stub == NULL) {
112112
LOG(ERROR) << "[MYSQL PACK] get prepare statement with NULL";
113113
return false;
@@ -284,7 +284,7 @@ ParseError HandlePrepareStatement(const InputResponse* msg,
284284
ParseError parseCode = PARSE_OK;
285285
butil::IOBuf buf;
286286
butil::Status st;
287-
auto stub = ControllerPrivateAccessor(cntl).mysql_stmt();
287+
auto stub = static_cast<MysqlStatementStub*>(ControllerPrivateAccessor(cntl).session_data());
288288
auto stmt = stub->stmt();
289289
if (stmt == NULL || stmt->param_count() != ok.param_count()) {
290290
LOG(ERROR) << "[MYSQL PACK] stmt can't be NULL";
@@ -359,6 +359,16 @@ ParseResult ParseMysqlMessage(butil::IOBuf* source,
359359
return MakeParseError(PARSE_ERROR_NOT_ENOUGH_DATA);
360360
}
361361
if (stmt_type == MYSQL_NEED_PREPARE) {
362+
// A failed PREPARE (e.g. ER_PARSE_ERROR 1064) comes back as a normal
363+
// ERR packet. Deliver it to the caller like any other error response
364+
// and keep the connection open -- matching the command path and other
365+
// protocols (redis, baidu_std). Only a successful prepare proceeds to
366+
// pack and send the COM_STMT_EXECUTE.
367+
if (!msg->response.reply(0).is_prepare_ok()) {
368+
msg->id_wait = pi.id_wait;
369+
socket->release_parsing_context();
370+
return MakeMessage(msg);
371+
}
362372
// store stmt_id, make execute header.
363373
ParseError err = HandlePrepareStatement(msg, socket, &pi);
364374
if (err != PARSE_OK) {
@@ -426,10 +436,11 @@ void SerializeMysqlRequest(butil::IOBuf* buf,
426436
if (!rr->SerializeTo(buf)) {
427437
return cntl->SetFailed(EREQUEST, "Fail to serialize MysqlRequest");
428438
}
429-
// mysql protocol don't use pipelined count to verify the end of a response, so pipelined count
430-
// is meanless, but we can use it help us to distinguish mysql reply type. In mysql protocol, we
431-
// can't distinguish OK and PreparedOk, so we set pipelined count to 2 to let parse function to
432-
// parse PreparedOk reply
439+
// mysql doesn't use pipelined_count to verify the end of a response; instead we
440+
// reuse it as a MysqlStmtType tag so the parse function knows which reply shape
441+
// to expect (OK and PrepareOk are otherwise indistinguishable). Default to
442+
// MYSQL_NORMAL_STATEMENT (1); it is upgraded to MYSQL_PREPARED_STATEMENT (2)
443+
// below when the request carries a prepared statement.
433444
ControllerPrivateAccessor accessor(cntl);
434445
accessor.set_pipelined_count(MYSQL_NORMAL_STATEMENT);
435446

@@ -439,7 +450,7 @@ void SerializeMysqlRequest(butil::IOBuf* buf,
439450
}
440451
auto st = rr->get_stmt();
441452
if (st != NULL) {
442-
accessor.set_mysql_stmt(st);
453+
accessor.set_session_data(rr->get_stmt());
443454
accessor.set_pipelined_count(MYSQL_PREPARED_STATEMENT);
444455
}
445456
if (FLAGS_mysql_verbose) {

0 commit comments

Comments
 (0)