wasphin commented on code in PR #3531:
URL: https://github.com/apache/brpc/pull/3531#discussion_r3951461861
##########
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:
Reworked in 0cdc618a. This test intentionally has one client, one server,
and synchronous calls, so only one request can be in flight and cannot be
re-entered concurrently. Adding concurrency handling only for the stream-id
container would be incomplete because , , and the request-side assertions are
also single-request state. The lock-based follow-up was therefore removed,
keeping the test aligned with its actual scope.
##########
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 close-result assertions are retained in 0cdc618a for both response and
request streams. currently returns 0 on every path, so these assertions pin
the expected cleanup contract rather than changing synchronization behavior.
##########
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:
Reworked in 0cdc618a. The public member is retained as test fixture state
because this test deliberately executes one synchronous request at a time.
Introducing a private locked accessor would suggest partial concurrent-request
support while the other observed fields and assertions remain single-request
state.
--
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]
