Copilot commented on code in PR #3631:
URL: https://github.com/apache/brpc/pull/3631#discussion_r4322795797
##########
src/brpc/policy/mongo_protocol.cpp:
##########
@@ -212,15 +229,13 @@ 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());
+ mongo_done->cntl.set_mongo_session_data(msg->context.get());
Review Comment:
`MongoInputMessage` now owns the only explicit strong reference
(`intrusive_ptr`) to the `MongoContext`, but
`set_mongo_session_data(msg->context.get())` passes a raw pointer. Once
`ProcessMongoRequest` returns, `msg` is destroyed and may drop the last
reference, which can make the controller hold a dangling pointer. To preserve
lifetime, keep a strong reference alive for at least as long as the
controller/request processing needs it (e.g., store an
`intrusive_ptr<MongoContext>` inside `SendMongoResponse` / controller-private
data, or change `set_mongo_session_data` to accept/store an `intrusive_ptr`
instead of a raw pointer).
##########
src/brpc/policy/mongo_protocol.cpp:
##########
@@ -161,13 +173,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 = new MongoInputMessage;
Review Comment:
This replaces `policy::MostCommonMessage::Get()` (which is commonly used as
a pooling/low-allocation path) with a per-request heap allocation. For
Mongo-heavy workloads this can increase allocator pressure. Consider adding a
pooling strategy for `MongoInputMessage` (similar to `MostCommonMessage`) or
reusing an existing pooled message type and extending it with the needed
`intrusive_ptr<MongoContext>` storage.
##########
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:
`DebugSocket()` holds `_parsing_context_mutex` while calling a virtual
`Describe()` implementation, which can run arbitrary/slow code. Because
`reset_parsing_context()` / `release_parsing_context()` now also take the same
mutex, a slow `/sockets/<id>` description can block parsing-context transitions
for that socket (and potentially contribute to latency spikes if the endpoint
is hit frequently). If feasible, prefer a design that does not hold the mutex
across the virtual call (e.g., make parsing contexts ref-counted so DebugSocket
can take a safe reference under the lock and then describe after unlocking, or
capture a minimal snapshot of describable state under lock and format it
outside).
##########
src/brpc/policy/mongo_protocol.cpp:
##########
@@ -110,6 +109,19 @@ void SendMongoResponse::Run() {
}
}
+// A mongo request and the context of its connection. The context is
+// referenced when the request is cut, because the bthread parsing messages
+// may reset the parsing context of the socket before the request is
+// processed in another bthread.
+struct MongoInputMessage : public InputMessageBase {
+ butil::IOBuf meta;
+ butil::IOBuf payload;
+ butil::intrusive_ptr<MongoContext> context;
+
+ // @InputMessageBase
+ void DestroyImpl() { delete this; }
Review Comment:
`DestroyImpl` should be marked `override` to ensure it correctly matches the
`InputMessageBase` virtual interface and avoids silent signature mismatches.
Updating it to `void DestroyImpl() override` makes the intent explicit and lets
the compiler catch errors.
--
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]
