This is an automated email from the ASF dual-hosted git repository.
chenBright pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 3c3e4ff8 fix(brpc): skip backup when retry budget is exhausted (#3688)
3c3e4ff8 is described below
commit 3c3e4ff88a8d625fb998fa5622c8bf9c6e5d6936
Author: darion-yaphet <[email protected]>
AuthorDate: Mon Nov 2 19:12:13 2026 +0800
fix(brpc): skip backup when retry budget is exhausted (#3688)
Keep the in-flight RPC alive when the backup timer fires without remaining
retry budget. Restore the original overall deadline instead of terminating with
EBACKUPREQUEST.
Cover zero and exhausted retry budgets across naming-service and direct
channels, synchronous and asynchronous calls, and finite and infinite timeouts.
Document the behavior in both client guides.
---
docs/cn/client.md | 2 ++
docs/en/client.md | 2 ++
src/brpc/controller.cpp | 13 ++++++--
test/brpc_channel_unittest.cpp | 70 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 85 insertions(+), 2 deletions(-)
diff --git a/docs/cn/client.md b/docs/cn/client.md
index d534bc1d..3a5252de 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 273da18e..cde6b9d2 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 4fff9fd2..ccfb8392 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 4374e0c8..efba8b6e 100644
--- a/test/brpc_channel_unittest.cpp
+++ b/test/brpc_channel_unittest.cpp
@@ -2239,6 +2239,57 @@ protected:
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<butil::atomic<int>>(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<brpc::Controller*>(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 {
@@ -3436,6 +3487,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;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
