thweetkomputer opened a new pull request, #3498: URL: https://github.com/apache/brpc/pull/3498
### What problem does this PR solve? Issue Number: N/A Problem Summary: `RedisService` has no server-side connection limit. `max_concurrency` limits requests only after protocol parsing, so idle clients can consume all available connection resources. On an SSL listener, accepting excess clients into brpc also allows them to enter the comparatively expensive TLS handshake before an application can reject them. ### What is changed and the side effects? Changed: - Add `ServerOptions.redis_max_connections` (`0` keeps the existing unlimited behavior). - Reserve connection slots atomically in `Acceptor`, before creating a brpc `Socket`, so idle clients count and concurrent accepts cannot exceed the limit. - Require a non-zero limit to use a dedicated Redis public listener: `redis_service` set, `enabled_protocols="redis"`, builtin services disabled, and no RPC or other protocol services on that `Server`. - Return `-ERR max number of clients reached` to excess plaintext clients. If SSL is configured, close the accepted fd immediately without creating a brpc `Socket` or starting TLS. - Keep internal acceptors and other `Server` instances unlimited, and expose the cumulative rejection count as `ServerStatistics.rejected_redis_connection_count`. - Document the option in the English and Chinese server guides and add regression tests for listener isolation, plaintext rejection, independent RPC availability, slot recovery, and pre-TLS rejection. Side effects: - Performance effects: accepted connections now perform one relaxed atomic increment/decrement; limited listeners use a relaxed CAS for admission. There is no request hot-path cost. - Breaking backward compatibility: the default behavior remains unlimited and existing source configuration is unchanged. The new public fields change the in-memory layouts of `ServerOptions` and `ServerStatistics`, so applications using a prebuilt shared brpc library must rebuild with the updated headers and library. --- ### Check List: Tests / Checks: - [x] `cmake --build build --target brpc_server_unittest --parallel 4` - [x] The three new Redis connection-limit tests pass. - [x] `ServerTest.close_idle_connections` passes. - [ ] The complete `brpc_server_unittest` is not clean in this local environment: the existing timing-sensitive overload assertions in `ServerTest.http_error_code` and `ServerTest.max_concurrency` observe a successful third RPC instead of overload rejection. The focused tests above pass; this PR does not change request concurrency code. - [x] `git diff --check upstream/master...HEAD` Code / Documentation: - [x] The code follows the repository style. - [x] Public API behavior and the accept/TLS invariants are documented. - [x] The final diff was reviewed against the merge base. Reviewer focus: - Whether requiring a dedicated Redis listener is the desired public contract. Protocol identification happens after TLS, so enforcing this invariant is what lets the acceptor reject excess TLS connections before authentication without affecting RPC listeners. - The relaxed atomic slot lifecycle: reserve immediately after `accept`, release on `Socket::Create` failure or `BeforeRecycle`. Rollback: Set `redis_max_connections` to `0` to retain unlimited admission, or revert this commit to remove the API and accounting fields. -- 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]
