Copilot commented on code in PR #3714:
URL: https://github.com/apache/brpc/pull/3714#discussion_r4482876684
##########
test/brpc_input_messenger_unittest.cpp:
##########
@@ -60,6 +60,19 @@ class MessengerTest : public ::testing::Test{
};
};
+TEST_F(MessengerTest, non_protocol_handlers_respect_capacity) {
+ brpc::InputMessenger messenger(1);
+ const brpc::InputMessageHandler handler = {
+ brpc::policy::ParseHuluMessage,
+ EmptyProcessHuluRequest,
+ nullptr,
+ nullptr,
+ "dummy_hulu"
+ };
Review Comment:
This positional aggregate initialization is brittle: if
`InputMessageHandler` field order changes, the test may silently initialize the
wrong members. Prefer explicit member assignment (construct then set `parse`,
`process`, etc.) so the test remains correct even if the struct evolves.
##########
src/brpc/input_messenger.cpp:
##########
@@ -232,6 +232,10 @@ int InputMessenger::AddNonProtocolHandler(const
InputMessageHandler& handler) {
return -1;
}
const int index = _max_index.load(butil::memory_order_relaxed) + 1;
+ if (static_cast<size_t>(index) >= _capacity) {
+ LOG(ERROR) << "Can't add more handlers than " << _capacity;
Review Comment:
The error message is a bit ambiguous: it doesn’t specify that this is the
*non-protocol* handler capacity and doesn’t include helpful context (e.g.,
attempted index/current max). Consider improving it to explicitly mention
non-protocol handlers and include `index` and `_capacity` to make production
debugging easier (e.g., \"Non-protocol handler capacity exhausted (index=X,
capacity=Y)\").
--
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]
