Copilot commented on code in PR #3229:
URL: https://github.com/apache/brpc/pull/3229#discussion_r2851746317
##########
src/brpc/backup_request_policy.h:
##########
@@ -19,23 +19,48 @@
#ifndef BRPC_BACKUP_REQUEST_POLICY_H
#define BRPC_BACKUP_REQUEST_POLICY_H
+#include <memory> // std::unique_ptr
#include "brpc/controller.h"
namespace brpc {
+class BackupRateLimiter;
+
+// Policy that controls backup request behavior: when to send a backup
+// request and whether to suppress it based on rate limiting.
+//
+// Users can subclass this to implement custom backup request logic.
+// When used as-is (not subclassed), provides default behavior:
+// - GetBackupRequestMs(): returns the configured timeout
+// - DoBackup(): checks rate limiter if initialized, otherwise allows
+// - OnRPCEnd(): updates rate limiter statistics if initialized
+//
+// Rate limiting is an internal enhancement: call InitRateLimit() to enable
+// ratio-based suppression within a sliding time window.
class BackupRequestPolicy {
public:
- virtual ~BackupRequestPolicy() = default;
+ explicit BackupRequestPolicy(int32_t backup_request_ms = -1);
+ virtual ~BackupRequestPolicy();
+
+ // Non-copyable (rate limiter has internal bvar state).
+ BackupRequestPolicy(const BackupRequestPolicy&) = delete;
+ BackupRequestPolicy& operator=(const BackupRequestPolicy&) = delete;
- // Return the time in milliseconds in which another request
- // will be sent if RPC does not finish.
- virtual int32_t GetBackupRequestMs(const Controller* controller) const = 0;
+ virtual int32_t GetBackupRequestMs(const Controller* controller) const;
+ virtual bool DoBackup(const Controller* controller) const;
+ virtual void OnRPCEnd(const Controller* controller);
- // Return true if the backup request should be sent.
- virtual bool DoBackup(const Controller* controller) const = 0;
+ // Initialize rate limiting as an internal enhancement.
+ // When the ratio of backup requests to total requests reaches or exceeds
+ // max_backup_ratio within the sliding window, DoBackup() returns false.
+ // NOTE: The ratio is cached and refreshed every `update_interval_seconds`.
+ void InitRateLimit(double max_backup_ratio,
+ int window_size_seconds = 10,
+ int update_interval_seconds = 5);
- // Called when a rpc is end, user can collect call information to adjust
policy.
- virtual void OnRPCEnd(const Controller* controller) = 0;
+private:
+ int32_t _backup_request_ms;
+ std::unique_ptr<BackupRateLimiter> _rate_limiter;
};
Review Comment:
Changing `BackupRequestPolicy` from a pure interface to a concrete class
with data members (`_backup_request_ms`, `_rate_limiter`) changes its
size/layout and introduces out-of-line symbols (ctor/dtor). Because this header
is user-facing, this is an ABI-breaking change for users who link against brpc
as a shared library and have their own derived `BackupRequestPolicy` compiled
separately. If ABI stability matters, consider keeping the interface class
unchanged and introducing a separate internal default/rate-limited
implementation class used by `Channel`.
##########
test/brpc_channel_unittest.cpp:
##########
@@ -2803,6 +2806,104 @@ TEST_F(ChannelTest, backup_request_policy) {
}
}
+TEST_F(ChannelTest, backup_request_rate_limited) {
+ ASSERT_EQ(0, StartAccept(_ep));
+
+ // Test 1: Policy is created when backup_request_max_ratio > 0
+ {
+ brpc::ChannelOptions opt;
+ opt.backup_request_ms = 10;
+ opt.backup_request_max_ratio = 0.3;
+ brpc::Channel channel;
+ ASSERT_EQ(0, channel.Init(_ep, &opt));
+ ASSERT_TRUE(channel.options().backup_request_policy != NULL);
+ }
+
+ // Test 2: No policy when ratio is disabled (-1)
+ {
+ brpc::ChannelOptions opt;
+ opt.backup_request_ms = 10;
+ opt.backup_request_max_ratio = -1;
+ brpc::Channel channel;
+ ASSERT_EQ(0, channel.Init(_ep, &opt));
+ ASSERT_TRUE(channel.options().backup_request_policy == NULL);
+ }
+
+ // Test 3: Custom policy is preserved, not replaced
+ {
+ brpc::ChannelOptions opt;
+ opt.backup_request_ms = 10;
+ opt.backup_request_max_ratio = 0.3;
+ opt.backup_request_policy = &_backup_request_policy;
+ brpc::Channel channel;
+ ASSERT_EQ(0, channel.Init(_ep, &opt));
+ ASSERT_EQ(&_backup_request_policy,
+ channel.options().backup_request_policy);
+ }
+
+ // Test 4: No policy when backup_request_ms < 0 (backup disabled)
+ {
+ brpc::ChannelOptions opt;
+ opt.backup_request_ms = -1;
+ opt.backup_request_max_ratio = 0.3;
+ brpc::Channel channel;
+ ASSERT_EQ(0, channel.Init(_ep, &opt));
+ ASSERT_TRUE(channel.options().backup_request_policy == NULL);
+ }
+
+ // Test 5: Functional — rate limiting reduces backup requests.
+ // Use short update interval so ratio refreshes on every DoBackup call.
+ // Save/restore gflags outside the inner scope so cleanup always runs.
+ const int32_t saved_interval =
+ brpc::FLAGS_backup_request_ratio_update_interval_s;
+ const int32_t saved_window =
+ brpc::FLAGS_backup_request_ratio_window_size_s;
+ brpc::FLAGS_backup_request_ratio_update_interval_s = 0;
Review Comment:
The test sets `FLAGS_backup_request_ratio_update_interval_s = 0` to force
refresh on every call, but the flag help text says the interval “Must be >= 1.”
Either update the flag’s contract/validation to allow 0 (and document it), or
adjust the test to use a supported value and avoid depending on an undocumented
configuration.
```suggestion
brpc::FLAGS_backup_request_ratio_update_interval_s = 1;
```
##########
src/brpc/channel.cpp:
##########
@@ -242,6 +250,28 @@ int Channel::InitChannelOptions(const ChannelOptions*
options) {
if (!cg.empty() && (::isspace(cg.front()) || ::isspace(cg.back()))) {
butil::TrimWhitespace(cg, butil::TRIM_ALL, &cg);
}
+
+ // Create rate-limited backup policy if configured.
+ // Per-channel option takes precedence over the global gflag.
+ double max_ratio = _options.backup_request_max_ratio;
+ if (max_ratio <= 0) {
Review Comment:
The precedence logic prevents a channel from explicitly disabling rate
limiting when the global `FLAGS_backup_request_max_ratio` is enabled:
`backup_request_max_ratio <= 0` falls back to the gflag, so setting
`ChannelOptions.backup_request_max_ratio = -1` won’t override a positive global
value. This contradicts the stated “per-channel override” behavior and the
comment that `-1 means no limit`. Consider treating `-1` as an explicit disable
(no fallback), and using a distinct sentinel for “inherit global” (or document
that `-1` means inherit).
```suggestion
// 0 means "inherit global"; negative (e.g. -1) means "no limit" /
// explicitly disable rate limiting for this channel.
if (max_ratio == 0) {
```
##########
src/brpc/backup_request_policy.cpp:
##########
@@ -0,0 +1,151 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "brpc/backup_request_policy.h"
+
+#include <gflags/gflags.h>
+#include "brpc/reloadable_flags.h"
+#include "bvar/reducer.h"
+#include "bvar/window.h"
+#include "butil/atomicops.h"
+#include "butil/time.h"
+
+namespace brpc {
+
+DEFINE_double(backup_request_max_ratio, -1,
+ "Maximum ratio of backup requests to total requests. "
+ "Value in (0, 1]. -1 means no limit (default). Can be overridden "
+ "per-channel via ChannelOptions.backup_request_max_ratio. "
+ "Note: takes effect at Channel::Init() time; changing this flag "
+ "at runtime does not affect already-created channels.");
+
+static bool validate_backup_request_max_ratio(const char*, double v) {
+ if (v < 0) return true; // negative means disabled
+ if (v > 0 && v <= 1.0) return true;
+ LOG(ERROR) << "Invalid backup_request_max_ratio=" << v
+ << ", must be -1 (disabled) or in (0, 1]";
+ return false;
+}
+BRPC_VALIDATE_GFLAG(backup_request_max_ratio,
+ validate_backup_request_max_ratio);
+
+DEFINE_int32(backup_request_ratio_window_size_s, 10,
+ "Window size in seconds for computing the backup request ratio. "
+ "Must be >= 1.");
+
+DEFINE_int32(backup_request_ratio_update_interval_s, 5,
+ "Interval in seconds between ratio cache updates. Must be >= 1.");
+
Review Comment:
`backup_request_ratio_window_size_s` and
`backup_request_ratio_update_interval_s` are documented as “Must be >= 1”, but
there’s no validator/clamp for either flag, and `BackupRateLimiter` will accept
0/negative values (potentially creating a 0-sized window or a 0µs update
interval). Add `BRPC_VALIDATE_GFLAG` validation (or clamp in `InitRateLimit`)
to enforce the documented constraints, and align the docs if 0 is intended to
be supported.
```suggestion
static bool validate_backup_request_ratio_window_size_s(const char*, int v) {
if (v >= 1) {
return true;
}
LOG(ERROR) << "Invalid backup_request_ratio_window_size_s=" << v
<< ", must be >= 1";
return false;
}
BRPC_VALIDATE_GFLAG(backup_request_ratio_window_size_s,
validate_backup_request_ratio_window_size_s);
DEFINE_int32(backup_request_ratio_update_interval_s, 5,
"Interval in seconds between ratio cache updates. Must be >= 1.");
static bool validate_backup_request_ratio_update_interval_s(const char*, int
v) {
if (v >= 1) {
return true;
}
LOG(ERROR) << "Invalid backup_request_ratio_update_interval_s=" << v
<< ", must be >= 1";
return false;
}
BRPC_VALIDATE_GFLAG(backup_request_ratio_update_interval_s,
validate_backup_request_ratio_update_interval_s);
```
##########
test/brpc_channel_unittest.cpp:
##########
@@ -2803,6 +2806,104 @@ TEST_F(ChannelTest, backup_request_policy) {
}
}
+TEST_F(ChannelTest, backup_request_rate_limited) {
+ ASSERT_EQ(0, StartAccept(_ep));
+
+ // Test 1: Policy is created when backup_request_max_ratio > 0
+ {
+ brpc::ChannelOptions opt;
+ opt.backup_request_ms = 10;
+ opt.backup_request_max_ratio = 0.3;
+ brpc::Channel channel;
+ ASSERT_EQ(0, channel.Init(_ep, &opt));
+ ASSERT_TRUE(channel.options().backup_request_policy != NULL);
+ }
+
+ // Test 2: No policy when ratio is disabled (-1)
+ {
+ brpc::ChannelOptions opt;
+ opt.backup_request_ms = 10;
+ opt.backup_request_max_ratio = -1;
+ brpc::Channel channel;
+ ASSERT_EQ(0, channel.Init(_ep, &opt));
+ ASSERT_TRUE(channel.options().backup_request_policy == NULL);
+ }
+
+ // Test 3: Custom policy is preserved, not replaced
+ {
+ brpc::ChannelOptions opt;
+ opt.backup_request_ms = 10;
+ opt.backup_request_max_ratio = 0.3;
+ opt.backup_request_policy = &_backup_request_policy;
+ brpc::Channel channel;
+ ASSERT_EQ(0, channel.Init(_ep, &opt));
+ ASSERT_EQ(&_backup_request_policy,
+ channel.options().backup_request_policy);
+ }
+
+ // Test 4: No policy when backup_request_ms < 0 (backup disabled)
+ {
+ brpc::ChannelOptions opt;
+ opt.backup_request_ms = -1;
+ opt.backup_request_max_ratio = 0.3;
+ brpc::Channel channel;
+ ASSERT_EQ(0, channel.Init(_ep, &opt));
+ ASSERT_TRUE(channel.options().backup_request_policy == NULL);
+ }
+
+ // Test 5: Functional — rate limiting reduces backup requests.
+ // Use short update interval so ratio refreshes on every DoBackup call.
+ // Save/restore gflags outside the inner scope so cleanup always runs.
+ const int32_t saved_interval =
+ brpc::FLAGS_backup_request_ratio_update_interval_s;
+ const int32_t saved_window =
+ brpc::FLAGS_backup_request_ratio_window_size_s;
Review Comment:
This test mutates global gflags and restores them only at the end of the
test. If any `ASSERT_*` inside the flagged section fails, the function returns
early and the restore lines won’t run, potentially impacting subsequent tests.
Use an RAII guard / scope-exit helper to restore the flags in a destructor so
restoration happens even on early returns.
```suggestion
// Save/restore gflags outside the inner scope so cleanup always runs.
struct BackupRequestFlagsGuard {
int32_t saved_interval;
int32_t saved_window;
BackupRequestFlagsGuard(int32_t interval, int32_t window)
: saved_interval(interval), saved_window(window) {}
~BackupRequestFlagsGuard() {
brpc::FLAGS_backup_request_ratio_update_interval_s =
saved_interval;
brpc::FLAGS_backup_request_ratio_window_size_s = saved_window;
}
};
const int32_t saved_interval =
brpc::FLAGS_backup_request_ratio_update_interval_s;
const int32_t saved_window =
brpc::FLAGS_backup_request_ratio_window_size_s;
BackupRequestFlagsGuard guard(saved_interval, saved_window);
```
##########
src/brpc/channel.cpp:
##########
@@ -176,6 +180,10 @@ Channel::~Channel() {
int Channel::InitChannelOptions(const ChannelOptions* options) {
+ // Reset any previously created internal backup policy so that
+ // re-initialization (calling Init multiple times) is clean.
Review Comment:
`InitChannelOptions` resets `_options._owned_backup_policy` but does not
clear `_options.backup_request_policy` when it was pointing at the owned
policy. If the channel is re-initialized with `options == NULL`, `_options` is
not overwritten, so `backup_request_policy` can become a dangling pointer (UAF)
after the reset. Clear `backup_request_policy` when it equals the owned policy
before resetting, or rebuild `_options` from a fresh `ChannelOptions()` on
re-init so the pointer can’t outlive the owned object.
```suggestion
// re-initialization (calling Init multiple times) is clean.
if (_options.backup_request_policy ==
_options._owned_backup_policy.get()) {
_options.backup_request_policy = NULL;
}
```
--
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]
