thweetkomputer commented on code in PR #3498:
URL: https://github.com/apache/brpc/pull/3498#discussion_r3975102703
##########
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:
This fd is owned by the caller's `butil::fd_guard in_fd(accept(...))` in
`OnNewConnectionsUntilEAGAIN()`. `RejectRedisConnection(int fd)` only borrows
the descriptor. After it returns, the caller executes `continue`, which
destroys `in_fd`; `fd_guard::~fd_guard()` calls `::close()` for every
non-negative fd. This covers both the SSL early return and the plaintext
best-effort send path.
Ownership is released only after a successful `Socket::Create()`, which the
rejection path never reaches. Adding a raw `close(fd)` here would therefore
cause a double close and could close a descriptor reused by another thread.
Verified on f6ed17a5: rebuilt `brpc_server_unittest` and ran the
dedicated-listener, plaintext/dynamic-limit, pre-TLS rejection, and
idle-connection tests; all four passed. In particular,
`reject_redis_connection_before_tls_handshake` asserts that the rejected client
receives EOF without sending a ClientHello. No fd-lifecycle change is needed.
##########
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 a `butil::fd_guard`, not a raw int. It is declared inside the
`while` loop, so this `continue` runs its destructor, which closes the accepted
fd. The guard remains the owner until `in_fd.release()` after successful
`Socket::Create()`; that ownership transfer is not reached on rejection.
Thus the existing rejection path already closes the connection. Adding
`close(in_fd)` before `continue`, or closing it inside
`RejectRedisConnection()`, would double-close the descriptor. The existing
pre-TLS rejection test also verifies peer-visible EOF and passed locally on
f6ed17a5.
--
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]
