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:
Done in bcba685d. `Echo` now collects the accepted ids into a local
`brpc::StreamIds` and appends them to a private `_response_streams` under a
`bthread::Mutex`; the per-request `clear()` is gone.
For the record, the race as described cannot happen in this test today.
`stub.Echo(&cntl, &request, &response, nullptr)` is synchronous —
`Channel::CallMethod` joins on the correlation id when `done == nullptr`
(`src/brpc/channel.cpp:656`) — so there is never more than one in-flight
request and `Echo` is not re-entered concurrently. The cross-thread read was
also already ordered by the release/acquire pair on `accepted_streams`.
The change is still worth making: it makes the ordering locally evident
instead of resting on an argument about the RPC round trip, and it stays
correct if this test ever grows an async call or a second concurrent client.
##########
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:
Done in bcba685d — both the response and the request close loops now use
`ASSERT_EQ(0, brpc::StreamClose(stream_id))`.
One clarification on the framing: `StreamClose` cannot currently fail. It
forwards to `Stream::SetFailed` -> `SetFailedV`, which returns 0 on every path,
including the "don't care recycled stream" early return
(`src/brpc/stream.cpp:879-888`, `982-984`). So the assertion is not guarding
against a masked failure — it pins the contract that close must return 0 here.
##########
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:
Done in bcba685d. `_response_streams` is private now, and the test drains it
through `TakeResponseStreams()`, which swaps the vector out under the same
`bthread::Mutex` that `Echo` takes when appending.
--
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]
