diff --git a/docs/cn/client.md b/docs/cn/client.md index d534bc1d62..3a5252de33 100755 --- a/docs/cn/client.md +++ b/docs/cn/client.md @@ -721,6 +721,8 @@ options.retry_policy = &g_my_retry_policy; 工作机制如下：如果response没有在backup_request_ms内返回，则发送另外一个请求，哪个先回来就取哪个。新请求会被尽量送到不同的server。注意如果backup_request_ms大于超时，则backup request总不会被发送。backup request会消耗一次重试次数。backup request不意味着server端cancel。 +如果没有剩余的重试次数，则跳过backup request，继续等待当前请求完成或整体RPC超时。 + ChannelOptions.backup_request_ms影响该Channel上所有RPC，单位毫秒，默认值-1（表示不开启）。Controller.set_backup_request_ms()可修改某次RPC的值。 用户可以通过继承[brpc::BackupRequestPolicy](https://github.com/apache/brpc/blob/master/src/brpc/backup_request_policy.h)自定义策略（backup_request_ms和熔断backup request）。 比如根据延时调节backup_request_ms或者根据错误率熔断部分backup request。 diff --git a/docs/en/client.md b/docs/en/client.md index 273da18e8a..cde6b9d24d 100644 --- a/docs/en/client.md +++ b/docs/en/client.md @@ -601,6 +601,8 @@ If the server does not respond and connection is good, retry is not triggered. I How it works: If response does not return within the timeout specified by backup_request_ms, send another request, take whatever the first returned. New request will be sent to a different server that never tried before by best efforts. NOTE: If backup_request_ms is greater than timeout_ms, backup request will never be sent. backup request consumes one retry. backup request does not imply a server-side cancellation. +If no retry budget remains, the backup request is skipped. The in-flight request continues until it completes or the overall RPC timeout expires. + ChannelOptions.backup_request_ms affects all RPC via the Channel, unit is milliseconds, Default value is -1(disabled), Controller.set_backup_request_ms() overrides value for one RPC. ### Timeout is not reached diff --git a/src/brpc/controller.cpp b/src/brpc/controller.cpp index 4fff9fd2f4..ccfb8392fc 100644 --- a/src/brpc/controller.cpp +++ b/src/brpc/controller.cpp @@ -869,12 +869,14 @@ void Controller::OnVersionedRPCReturned(const CompletionInfo& info, return; } + const bool retry_limit_reached = _current_call.nretry >= _max_retry; if ((!_error_code && _retry_policy == nullptr) || - _current_call.nretry >= _max_retry) { + (_error_code != EBACKUPREQUEST && retry_limit_reached)) { goto END_OF_RPC; } if (_error_code == EBACKUPREQUEST) { - if (nullptr != _backup_request_policy && !_backup_request_policy->DoBackup(this)) { + if (!retry_limit_reached && nullptr != _backup_request_policy && + !_backup_request_policy->DoBackup(this)) { // No need to do backup request. _error_code = saved_error; CHECK_EQ(0, bthread_id_unlock(info.id)); @@ -893,6 +895,13 @@ void Controller::OnVersionedRPCReturned(const CompletionInfo& info, SetFailed(rc, "Fail to add timer"); goto END_OF_RPC; } + if (retry_limit_reached) { + // The backup timer is not an RPC failure. Keep the in-flight call + // and its original deadline even when no retry budget remains. + _error_code = saved_error; + CHECK_EQ(0, bthread_id_unlock(info.id)); + return; + } if (!SingleServer()) { if (_accessed == nullptr) { _accessed = ExcludedServers::Create( diff --git a/test/brpc_channel_unittest.cpp b/test/brpc_channel_unittest.cpp index f6155cbdb5..add7c48ac9 100644 --- a/test/brpc_channel_unittest.cpp +++ b/test/brpc_channel_unittest.cpp @@ -2238,6 +2238,57 @@ class ChannelTest : public ::testing::Test{ StopAndJoin(); } + void TestBackupRequestWithoutRetryBudget(bool single_server, bool async, + int timeout_ms, + bool consume_retry_first) { + ASSERT_EQ(0, StartAccept(_ep)); + brpc::Channel channel; + SetUpChannel(&channel, single_server, false); + + // Establish the connection before testing backup/retry ordering. + test::EchoRequest req; + test::EchoResponse res; + req.set_message(__FUNCTION__); + brpc::Controller warmup; + warmup.set_timeout_ms(1000); + CallMethod(&channel, &warmup, &req, &res, async); + ASSERT_FALSE(warmup.Failed()) << warmup.ErrorText(); + + auto attempts = std::make_shared>(0); + _svc.SetMockFunc([attempts, consume_retry_first]( + google::protobuf::RpcController* controller, + const test::EchoRequest*, test::EchoResponse*, + google::protobuf::Closure*) { + if (attempts->fetch_add(1, butil::memory_order_relaxed) == 0 && + consume_retry_first) { + static_cast(controller)->SetFailed( + brpc::EOVERCROWDED, "Consume the only retry before backup"); + return; + } + bthread_usleep(100000); + }); + + brpc::Controller cntl; + cntl.set_max_retry(consume_retry_first ? 1 : 0); + cntl.set_backup_request_ms(20); + cntl.set_timeout_ms(timeout_ms); + CallMethod(&channel, &cntl, &req, &res, async); + if (timeout_ms < 0 || timeout_ms > 100) { + EXPECT_EQ(0, cntl.ErrorCode()) << cntl.ErrorText(); + EXPECT_EQ("received " + req.message(), res.message()); + } else { + EXPECT_EQ(brpc::ERPCTIMEDOUT, cntl.ErrorCode()) << cntl.ErrorText(); + } + EXPECT_FALSE(cntl.has_backup_request()); + EXPECT_EQ(consume_retry_first ? 1 : 0, cntl.retried_count()); + // Also finish delayed server work when the client has timed out. + bthread_usleep(150000); + EXPECT_EQ(consume_retry_first ? 2 : 1, + attempts->load(butil::memory_order_relaxed)); + StopAndJoin(); + _svc.SetMockFunc({}); + } + class BackupRequestPolicyImpl : public brpc::BackupRequestPolicy { public: int32_t GetBackupRequestMs(const brpc::Controller*) const override { @@ -3396,6 +3447,25 @@ TEST_F(ChannelTest, backup_request_policy) { } } +TEST_F(ChannelTest, backup_request_without_retry_budget) { + const int timeouts_ms[] = {1000, 60, -1}; + for (bool single_server : {false, true}) { + for (bool async : {false, true}) { + for (int timeout_ms : timeouts_ms) { + for (bool consume_retry_first : {false, true}) { + SCOPED_TRACE(testing::Message() + << "single_server=" << single_server + << " async=" << async + << " timeout_ms=" << timeout_ms + << " consume_retry_first=" << consume_retry_first); + TestBackupRequestWithoutRetryBudget( + single_server, async, timeout_ms, consume_retry_first); + } + } + } + } +} + TEST_F(ChannelTest, selective_channel_ignores_late_subdone_after_timeout) { DelayedCloseEchoService service; brpc::Server server;