wwbmmm commented on code in PR #3631:
URL: https://github.com/apache/brpc/pull/3631#discussion_r4356564221
##########
src/brpc/policy/mongo_protocol.cpp:
##########
@@ -212,15 +239,15 @@ void ProcessMongoRequest(InputMessageBase* msg_base) {
const Server::MethodProperty *mp =
server_accessor.FindMethodPropertyByFullName(
srv_des->method(0)->full_name());
- MongoContextMessage* context_msg =
- dynamic_cast<MongoContextMessage*>(socket->parsing_context());
- if (nullptr == context_msg) {
+ if (nullptr == msg->context) {
LOG(WARNING) << "socket context wasn't set correctly";
return;
}
SendMongoResponse* mongo_done = new SendMongoResponse(server);
- mongo_done->cntl.set_mongo_session_data(context_msg->context());
+ // The controller takes its own strong reference of the context, which
+ // outlives this message.
+ mongo_done->cntl.set_mongo_session_data(msg->context.get());
Review Comment:
[replied by brpc-oncall robot] The refcount is incremented:
`Controller::set_mongo_session_data()` is `_mongo_session_data = data;`
(controller.cpp:1885), and `_mongo_session_data` is a
`butil::intrusive_ptr<MongoContext>` (controller.h:898).
`intrusive_ptr::operator=(T*)` constructs with `add_ref = true`, so the
controller takes its own strong reference. The comment at the call site now
states this explicitly.
##########
src/brpc/policy/mongo_protocol.cpp:
##########
@@ -161,13 +183,18 @@ ParseResult ParseMongoMessage(butil::IOBuf* source,
socket_context_msg = new MongoContextMessage(context);
socket->reset_parsing_context(socket_context_msg);
}
- policy::MostCommonMessage* msg = policy::MostCommonMessage::Get();
+ MongoInputMessage* msg = MongoInputMessage::Get();
source->cutn(&msg->meta, sizeof(buf));
size_t act_body_len = source->cutn(&msg->payload, body_len - sizeof(buf));
if (act_body_len != body_len - sizeof(buf)) {
CHECK(false); // Very unlikely, unless memory is corrupted.
return MakeParseError(PARSE_ERROR_TRY_OTHERS);
}
+ MongoContextMessage* context_msg =
+ dynamic_cast<MongoContextMessage*>(socket_context_msg);
+ if (nullptr != context_msg) {
+ msg->context = context_msg->context();
+ }
Review Comment:
[replied by brpc-oncall robot] This keeps the previous behavior: the same
`dynamic_cast` was done in `ProcessMongoRequest()` before. On the parsing
bthread `InputMessenger` resets the parsing context to null before trying
another protocol, so a successful mongo parse sees either null (and creates the
context here) or its own `MongoContextMessage`. Creating a fresh context when a
foreign one is found would hide a protocol mismatch instead of reporting it, so
the warning is intentional.
##########
src/brpc/socket.cpp:
##########
@@ -2427,17 +2427,20 @@ void Socket::DebugSocket(std::ostream& os, SocketId id)
{
os << idsizes[i];
}
os << '}';
- Destroyable* const parsing_context = ptr->parsing_context();
- Describable* parsing_context_desc =
dynamic_cast<Describable*>(parsing_context);
- if (parsing_context_desc) {
- os << "\nparsing_context=" << butil::class_name_str(*parsing_context)
<< '{';
- DescribeOptions opt;
- opt.verbose = true;
- IndentingOStream os2(os, 2);
- parsing_context_desc->Describe(os2, opt);
- os << '}';
- } else {
- os << "\nparsing_context=" << ShowObject(parsing_context);
+ {
+ BAIDU_SCOPED_LOCK(ptr->_parsing_context_mutex);
+ Destroyable* const parsing_context = ptr->parsing_context();
+ Describable* parsing_context_desc =
dynamic_cast<Describable*>(parsing_context);
+ if (parsing_context_desc) {
+ os << "\nparsing_context=" <<
butil::class_name_str(*parsing_context) << '{';
+ DescribeOptions opt;
+ opt.verbose = true;
+ IndentingOStream os2(os, 2);
+ parsing_context_desc->Describe(os2, opt);
+ os << '}';
+ } else {
+ os << "\nparsing_context=" << ShowObject(parsing_context);
+ }
Review Comment:
[replied by brpc-oncall robot] The mutex is held only while `/sockets/<id>`
describes that single socket; parsing-context transitions of other sockets are
unaffected. Ref-counting parsing contexts or snapshotting their state would be
a much larger change, and among the built-in contexts only `H2Context` is
`Describable`, whose `Describe()` just prints state under its own mutexes. I
added a note on `_parsing_context_mutex` that `Describe()` runs under the mutex
and must not reset the socket's parsing context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
