Copilot commented on code in PR #3531:
URL: https://github.com/apache/brpc/pull/3531#discussion_r3951092126
##########
test/brpc_streaming_rpc_unittest.cpp:
##########
@@ -1446,6 +1447,9 @@ TEST_F(StreamingRpcTest,
limit_streams_accepted_per_request) {
std::memory_order_acquire));
}
+ for (brpc::StreamId stream_id : service.response_streams) {
+ brpc::StreamClose(stream_id);
+ }
for (brpc::StreamId stream_id : request_streams) {
brpc::StreamClose(stream_id);
Review Comment:
The return value from `brpc::StreamClose` is ignored, which can mask cleanup
failures and make the test flaky/harder to diagnose. Consider asserting success
(or at least logging/asserting in debug) for both response and request stream
closes so failures are immediately visible.
##########
test/brpc_streaming_rpc_unittest.cpp:
##########
@@ -1388,7 +1388,7 @@ class MyServiceWithStreamCountLimit : public
test::EchoService {
brpc::Controller* cntl = static_cast<brpc::Controller*>(controller);
response->set_message(request->message());
- brpc::StreamIds response_streams;
+ response_streams.clear();
accept_result.store(
brpc::StreamAccept(response_streams, *cntl, nullptr),
std::memory_order_release);
Review Comment:
`response_streams` is now shared state on the service instance and is
mutated per-RPC. If the server handles multiple requests concurrently, this
introduces a data race (concurrent `clear()`/writes by `StreamAccept`).
Consider making the stored IDs per-request (e.g., keep a local
`brpc::StreamIds` and then append/copy into a shared collection under a mutex),
or guard `response_streams` with a lock.
##########
test/brpc_streaming_rpc_unittest.cpp:
##########
@@ -1398,6 +1398,7 @@ class MyServiceWithStreamCountLimit : public
test::EchoService {
std::atomic<int> accept_result{0};
std::atomic<size_t> accepted_streams{0};
+ brpc::StreamIds response_streams;
Review Comment:
`response_streams` is a public data member on the service and is accessed
directly by the test. To keep the service encapsulated (even in tests) and to
reduce accidental misuse, consider making it private and exposing a small
accessor (optionally returning a copy/snapshot under a lock if you address the
concurrency concern).
--
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]
