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 stub.Echo(..., nullptr) calls, so only one request can be in
flight and Echo cannot be re-entered concurrently. Adding concurrency handling
only for the stream-id container would be incomplete because accept_result,
accepted_streams, 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. StreamClose currently returns 0 on every Stream::SetFailedV
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 response_streams 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.
##########
test/brpc_streaming_rpc_unittest.cpp:
##########
@@ -1394,10 +1394,25 @@ class MyServiceWithStreamCountLimit : public
test::EchoService {
std::memory_order_release);
accepted_streams.store(
response_streams.size(), std::memory_order_release);
Review Comment:
Reworked in 0cdc618a by removing the lock-based state entirely. This test
has one client, one server, and synchronous stub.Echo(..., nullptr) calls, so
only one request can be in flight. Extending concurrency handling only to the
stream-id container would be misleading: accept_result, accepted_streams, and
the request-side assertions are also deliberately single-request state. The
test now keeps that explicit scope and only asserts both StreamClose calls.
--
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]
