Copilot commented on code in PR #3600:
URL: https://github.com/apache/brpc/pull/3600#discussion_r4256761555
##########
test/brpc_channel_unittest.cpp:
##########
@@ -3389,6 +3438,183 @@ TEST_F(ChannelTest,
selective_channel_ignores_late_subdone_after_timeout) {
server.Join();
}
+static void TestSelectiveLateSubDone(int terminal_error, int late_error,
+ bool backup_first, bool delete_in_done) {
+ SCOPED_TRACE(testing::Message() << "terminal_error=" << terminal_error
+ << " late_error=" << late_error
+ << " backup_first=" << backup_first
+ << " delete_in_done=" << delete_in_done);
+ SelectiveRetryPolicy retry_policy;
+ brpc::ChannelOptions options;
+ options.timeout_ms = -1;
+ options.max_retry = 3;
+ options.retry_policy = &retry_policy;
+ std::vector<DeferredSelectiveChannel::SubCall> calls;
+ brpc::SelectiveChannel channel;
+ ASSERT_EQ(0, channel.Init("rr", &options));
+ for (int i = 0; i < 2; ++i) {
+ ASSERT_EQ(0, channel.AddChannel(
+ new DeferredSelectiveChannel(&calls), nullptr));
+ }
+
+ std::unique_ptr<brpc::Controller> cntl(new brpc::Controller);
+ const brpc::CallId cid = cntl->call_id();
+ test::EchoRequest req;
+ test::EchoResponse res;
+ req.set_message("late subcall");
+ res.set_message("original response");
+ int done_count = 0;
+ auto on_done = [](int* count, std::unique_ptr<brpc::Controller>* owner,
+ bool destroy) {
+ ++*count;
+ if (destroy) {
+ owner->reset();
+ }
+ };
+ test::EchoService::Stub(&channel).Echo(
+ cntl.get(), &req, &res,
+ brpc::NewCallback(+on_done, &done_count, &cntl, delete_in_done));
+ ASSERT_EQ(1u, calls.size());
+
+ // Drive the backup and timeout/cancel paths under the same call-id lock
+ // as timer callbacks, without depending on timer scheduling or sleeps.
+ const brpc::Controller::CompletionInfo info = {cid, false};
+ ASSERT_EQ(0, bthread_id_trylock(cid, nullptr));
+ cntl->SetFailed(brpc::EBACKUPREQUEST, "trigger backup");
+ cntl->OnVersionedRPCReturned(info, false, 0);
+ ASSERT_EQ(2u, calls.size());
+ ASSERT_TRUE(cntl->has_backup_request());
+
+ const int first = backup_first ? 1 : 0;
+ if (late_error != 0) {
+ calls[first].cntl->SetFailed(late_error, "late subcall error");
+ }
+ static_cast<test::EchoResponse*>(calls[first].cntl->_response)
+ ->set_message("late response");
+ calls[first].cntl->response_attachment().append("late attachment");
+ cntl->response_attachment().append("original attachment");
+
+ ASSERT_EQ(0, bthread_id_trylock(cid, nullptr));
+ cntl->SetFailed(terminal_error, "main RPC finished");
+ cntl->OnVersionedRPCReturned(info, false, 0);
+ ASSERT_TRUE(cntl->is_ending_rpc());
+ ASSERT_EQ(nullptr, cntl->_lb.get());
+ ASSERT_EQ(0, done_count);
Review Comment:
This test asserts an internal invariant (`Controller::_lb` being cleared)
which is not part of the public/observable behavior and is likely to be fragile
across refactors (especially since the PR description notes newer
implementations may keep a separate LB reference in `Sender`). Consider
removing this assertion and validating the intended behavior via externally
observable effects only (e.g., no retries triggered, terminal error preserved,
response/attachment unchanged).
##########
test/brpc_channel_unittest.cpp:
##########
@@ -83,6 +83,55 @@ void MarkCalled(bool* called) {
*called = true;
}
+// Hold completed subcalls before invoking SelectiveChannel's SubDone. Their
+// call IDs stay locked, just as they do while Controller::EndRPC runs done,
+// so cancellation cannot replace an already received response.
+class DeferredSelectiveChannel : public brpc::ChannelBase {
+public:
+ struct SubCall {
+ brpc::Controller* cntl;
+ google::protobuf::Closure* done;
+
+ void Run() const {
+ const brpc::CallId cid = cntl->call_id();
+ done->Run();
+ // The final SubDone may have deleted its Controller.
+ EXPECT_EQ(0, bthread_id_unlock_and_destroy(cid));
+ }
+ };
+
+ explicit DeferredSelectiveChannel(std::vector<SubCall>* calls)
+ : _calls(calls) {}
+
+ void CallMethod(const google::protobuf::MethodDescriptor*,
+ google::protobuf::RpcController* controller,
+ const google::protobuf::Message*,
+ google::protobuf::Message* response,
+ google::protobuf::Closure* done) override {
+ brpc::Controller* cntl = static_cast<brpc::Controller*>(controller);
+ cntl->_response = response;
+ cntl->set_used_by_rpc();
+ CHECK_EQ(0, bthread_id_lock(cntl->call_id(), nullptr));
+ _calls->push_back({cntl, done});
+ }
+
+ int CheckHealth() override { return 0; }
+ int Weight() override { return 1; }
+
+private:
+ std::vector<SubCall>* _calls;
+};
+
+class SelectiveRetryPolicy : public brpc::RetryPolicy {
+public:
+ bool DoRetry(const brpc::Controller* cntl) const override {
+ ++calls;
+ return cntl->ErrorCode() == 5013;
+ }
+
+ mutable int calls = 0;
Review Comment:
`RetryPolicy::DoRetry` may be called concurrently in real usage, and
mutating `calls` inside a `const` method via a plain `int` introduces a
potential data race if the policy is ever shared across threads. Even though
these are tests, switching `calls` to `std::atomic<int>` (or avoiding shared
mutation entirely) would make the helper safer and more future-proof.
--
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]
