Copilot commented on code in PR #3498:
URL: https://github.com/apache/brpc/pull/3498#discussion_r3958351666
##########
src/brpc/acceptor.cpp:
##########
@@ -200,9 +214,61 @@ void Acceptor::Join() {
}
size_t Acceptor::ConnectionCount() const {
- // Notice that _socket_map may be modified concurrently. This actually
- // assumes that size() is safe to call concurrently.
- return _socket_map.size();
+ return _connection_count.load(butil::memory_order_relaxed);
+}
+
+size_t Acceptor::RejectedRedisConnectionCount() const {
+ return _rejected_redis_connection_count.load(butil::memory_order_relaxed);
+}
+
+bool Acceptor::TryAcquireRedisConnectionSlot() {
+ size_t count = _connection_count.load(butil::memory_order_relaxed);
+ do {
+ const size_t max_connections =
+ _redis_max_connections.load(butil::memory_order_relaxed);
+ if (max_connections != 0 && count >= max_connections) {
+ return false;
+ }
+ } while (!_connection_count.compare_exchange_weak(
+ count, count + 1, butil::memory_order_relaxed));
+ return true;
+}
+
+void Acceptor::SetRedisMaxConnections(size_t max_connections) {
+ // The limit controls only future numeric admission decisions and does not
+ // publish socket state, so a relaxed store is sufficient.
+ _redis_max_connections.store(
+ max_connections, butil::memory_order_relaxed);
+}
+
+void Acceptor::RejectRedisConnection(int fd) {
+ _rejected_redis_connection_count.fetch_add(
+ 1, butil::memory_order_relaxed);
+
+ // Reject SSL-capable listeners before doing any TLS work. Plaintext here
+ // would violate the TLS record protocol and could trigger an expensive
+ // handshake in a higher layer.
+ if (_ssl_ctx) {
+ return;
+ }
Review Comment:
For SSL-enabled listeners, this returns without closing `fd`, which
contradicts the PR contract (“close the accepted fd immediately”) and will
cause the connection to remain open until the peer closes (and leak the server
fd in the meantime). Even in the plaintext branch, the function sends a
response but never closes the connection afterward. Ensure `fd` is always
closed (for SSL: close immediately; for plaintext: send best-effort then close).
##########
src/brpc/server.cpp:
##########
@@ -870,6 +893,18 @@ int Server::StartInternal(const butil::EndPoint& endpoint,
const ServerOptions default_opt;
const ServerOptions& real_opt = opt ? *opt : default_opt;
+ // Admission happens before protocol parsing (and, importantly, before a
+ // TLS handshake), so it is only safe on a listener dedicated to Redis.
+ // Reject ambiguous configurations instead of accidentally limiting RPCs
+ // sharing the public port.
+ if (real_opt.redis_max_connections != 0 &&
+ !is_redis_only_public_listener(real_opt, service_count())) {
+ LOG(ERROR) << "redis_max_connections requires a Redis-only public "
+ "listener (redis_service set, enabled_protocols=redis, "
+ "no RPC or builtin services)";
Review Comment:
The validation in `is_redis_only_public_listener(...)` checks additional
constraints (e.g., `enabled_protocols` must be exactly `"redis"` and several
other `*_service` pointers must be `nullptr`), but the error message doesn’t
reflect the full set of reasons this can fail. Consider expanding the message
(or pointing to the docs/option name) so users can resolve configuration errors
without reading code.
##########
src/brpc/acceptor.cpp:
##########
@@ -275,7 +341,12 @@ void Acceptor::OnNewConnectionsUntilEAGAIN(Socket*
acception) {
acception->SetFailed(EINVAL, "Impossible! acception->user() MUST
be Acceptor");
return;
}
-
+
+ if (!am->TryAcquireRedisConnectionSlot()) {
+ am->RejectRedisConnection(in_fd);
+ continue;
+ }
Review Comment:
`in_fd` is not closed on the rejection path. Since no `Socket` is created,
nothing else will own/close this fd, which can leak file descriptors and leave
rejected clients connected indefinitely (also defeating the purpose of “reject
excess clients early”). Close the fd before `continue` (or make
`RejectRedisConnection` always close it).
##########
test/brpc_server_unittest.cpp:
##########
@@ -1424,6 +1425,159 @@ TEST_F(ServerTest, close_idle_connections) {
ASSERT_EQ(0ul, stat.connection_count);
}
+TEST_F(ServerTest, redis_connection_limit_requires_dedicated_listener) {
+ brpc::Server server;
+ EchoServiceImpl echo_service;
+ ASSERT_EQ(0, server.AddService(
+ &echo_service, brpc::SERVER_DOESNT_OWN_SERVICE));
+
+ brpc::ServerOptions opt;
+ opt.redis_service = new brpc::RedisService;
+ opt.redis_max_connections = 1;
+ opt.enabled_protocols = "redis";
+ opt.has_builtin_services = false;
+ const int rc = server.Start("127.0.0.1:0", &opt);
+ if (rc != 0) {
+ delete opt.redis_service;
+ opt.redis_service = nullptr;
+ }
+ EXPECT_EQ(-1, rc);
+}
+
+TEST_F(ServerTest, reject_redis_connections_over_limit) {
+ brpc::Server server;
+ brpc::ServerOptions opt;
+ opt.redis_service = new brpc::RedisService;
+ opt.redis_max_connections = 0;
+ opt.enabled_protocols = "redis";
+ opt.has_builtin_services = false;
+ ASSERT_EQ(0, server.Start("127.0.0.1:0", &opt));
+ ASSERT_EQ(0, server.SetRedisMaxConnections(1));
+
+ const butil::EndPoint ep = server.listen_address();
+ butil::fd_guard first_client(tcp_connect(ep, nullptr));
+ ASSERT_GT(first_client, 0);
Review Comment:
`tcp_connect` appears to return `-1` on failure; a valid fd can be `0` in
some test environments (e.g., if stdin was closed). Using `ASSERT_GT(fd, 0)`
can spuriously fail; prefer `ASSERT_GE(fd, 0)` (and similarly for the other
`ASSERT_GT(..., 0)` added in these tests).
--
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]
