Skip to content

feat: limit Redis server connections - #3498

Open
thweetkomputer wants to merge 7 commits into
apache:masterfrom
thweetkomputer:feature/redis-max-connections
Open

feat: limit Redis server connections#3498
thweetkomputer wants to merge 7 commits into
apache:masterfrom
thweetkomputer:feature/redis-max-connections

Conversation

@thweetkomputer

@thweetkomputer thweetkomputer commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

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).
  • Add Server::SetRedisMaxConnections() to atomically change the limit on a running Redis-only Server. Raising or disabling the limit affects subsequent accepts; lowering it does not close established connections.
  • Reserve connection slots atomically in Acceptor, before creating a brpc Socket, so idle clients count and concurrent accepts cannot exceed the limit.
  • Require the 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. A dedicated listener can start unlimited and enable the limit later.
  • Make a nonblocking, best-effort attempt to send -ERR max number of clients reached to excess plaintext clients, then close the connection. Backpressure or socket errors may prevent some or all of the response from being delivered. If SSL is configured, close the accepted fd immediately without creating a brpc Socket or starting TLS.
  • Guard MSG_NOSIGNAL and fall back to SO_NOSIGPIPE where available. If per-socket SIGPIPE suppression is unavailable or fails, omit the optional plaintext error and close the connection.
  • Share the dedicated-listener diagnostic between startup and runtime validation; use RAII in the invalid-configuration test and explicitly verify admission after slot recovery.
  • Keep internal acceptors and other Server instances unlimited, and expose the cumulative rejection count as ServerStatistics.rejected_redis_connection_count.
  • Document the startup option and runtime setter in the English and Chinese server guides and add regression tests for listener isolation, dynamic limit changes, plaintext rejection, independent RPC availability, slot recovery, and pre-TLS rejection. Plaintext rejection tests accumulate short TCP reads and verify EOF; fd ownership and best-effort delivery are documented beside the rejection code.

Side effects:

  • Performance effects: accepted connections perform relaxed atomic admission/accounting, including an atomic load of the current limit; runtime updates are a relaxed atomic store. 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:

  • macOS / SO_NOSIGPIPE fallback not built or run locally. Linux validation below exercises MSG_NOSIGNAL.

  • cmake --build build --target brpc_server_unittest --parallel 4

  • Focused tests pass: dedicated-listener validation, runtime enable/raise/lower behavior, RPC isolation, plaintext rejection with short reads and EOF, pre-TLS rejection, and ServerTest.close_idle_connections (4/4 tests; rerun on 2026-09-10 after the portability and slot-recovery follow-up).

  • The complete brpc_server_unittest was not rerun in this follow-up. The previously recorded full-suite run was 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.

  • git diff --check upstream/master...HEAD

Code / Documentation:

  • The code follows the repository style.
  • Public API behavior and the accept/TLS invariants are documented.
  • 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: load the mutable limit and reserve immediately after accept, release on Socket::Create failure or BeforeRecycle, and update the limit with a relaxed store.

Rollback:

Call SetRedisMaxConnections(0) (or start with redis_max_connections=0) to retain unlimited admission, or revert these commits to remove the API and accounting fields.

@thweetkomputer
thweetkomputer marked this pull request as ready for review August 28, 2026 04:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a server-side admission limit for Redis-only listeners to prevent idle clients (and, for SSL, expensive TLS handshakes) from consuming connection resources before request-level concurrency limits apply.

Changes:

  • Introduces ServerOptions.redis_max_connections and Server::SetRedisMaxConnections() to configure/update the Redis connection admission limit (default unlimited).
  • Enforces a “dedicated Redis listener” contract and reserves connection slots in Acceptor immediately after accept() (before Socket::Create() / TLS).
  • Adds stats (ServerStatistics.rejected_redis_connection_count), documentation updates (EN/CN), and unit tests covering rejection behavior and dynamic updates.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/brpc_server_unittest.cpp Adds regression tests for dedicated-listener validation, plaintext rejection, TLS pre-handshake rejection, and dynamic limit updates.
src/brpc/server.h Adds redis_max_connections, rejected_redis_connection_count, and Server::SetRedisMaxConnections() API surface.
src/brpc/server.cpp Validates dedicated Redis-only configuration, wires redis limit into acceptor startup, aggregates rejection stats, implements runtime setter.
src/brpc/acceptor.h Extends StartAccept signature and adds atomics/methods for connection slot accounting and rejection stats.
src/brpc/acceptor.cpp Implements slot reservation before socket creation, relaxed-atomic accounting, and plaintext/TLS rejection behavior.
docs/en/server.md Documents Redis connection limiting, dedicated listener requirements, runtime setter, and stats.
docs/cn/server.md Chinese documentation for the Redis connection limiting feature and behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/brpc/acceptor.cpp Outdated
…-connections

# Conflicts:
#	src/brpc/server.cpp
@wwbmmm
wwbmmm requested a lite review from Copilot September 8, 2026 03:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wwbmmm
wwbmmm requested a lite review from Copilot September 8, 2026 04:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wwbmmm
wwbmmm requested a lite review from Copilot September 8, 2026 05:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wwbmmm
wwbmmm requested a lite review from Copilot September 8, 2026 06:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wwbmmm
wwbmmm requested a lite review from Copilot September 8, 2026 07:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wwbmmm
wwbmmm requested a lite review from Copilot September 8, 2026 08:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wwbmmm
wwbmmm requested a lite review from Copilot September 8, 2026 09:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wwbmmm
wwbmmm requested a lite review from Copilot September 8, 2026 11:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wwbmmm
wwbmmm requested a lite review from Copilot September 8, 2026 12:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wwbmmm
wwbmmm requested a lite review from Copilot September 8, 2026 13:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Comment thread src/brpc/acceptor.cpp
Comment thread src/brpc/acceptor.cpp
Comment thread test/brpc_server_unittest.cpp Outdated
Comment thread src/brpc/server.cpp Outdated
Accept fd 0 in the Redis connection tests and describe every dedicated-listener requirement in startup and runtime errors.

Validation: brpc_server_unittest rebuilt; dedicated-listener, plaintext/dynamic-limit, pre-TLS rejection, and idle-connection tests passed (4/4). Diff whitespace check passed. Full suite not run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Comment thread src/brpc/acceptor.cpp
Comment thread src/brpc/acceptor.cpp
Comment thread test/brpc_server_unittest.cpp Outdated
Document that rejection borrows the fd from the accept loop guard, which closes it on continue. Describe nonblocking best-effort error delivery in both server guides. Accumulate short TCP reads in rejection tests and verify plaintext EOF.

Validation: rebuilt brpc_server_unittest; dedicated-listener, plaintext/dynamic-limit, pre-TLS rejection and idle-connection tests passed (4/4). Merge-base diff whitespace check passed. Full suite not run. Production behavior is unchanged.
@wwbmmm
wwbmmm requested a lite review from Copilot September 10, 2026 04:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Comment thread src/brpc/acceptor.cpp Outdated
Comment thread test/brpc_server_unittest.cpp
Comment thread test/brpc_server_unittest.cpp Outdated
Comment thread src/brpc/server.cpp Outdated
Comment thread test/brpc_server_unittest.cpp
Guard MSG_NOSIGNAL and use SO_NOSIGPIPE when available; omit the optional error if per-socket signal suppression is unavailable. Share listener validation diagnostics, use RAII based on actual Server ownership in the invalid-configuration test, and verify new admission after slot recovery.

Validation: cmake --build build --target brpc_server_unittest --parallel 4 succeeded. Dedicated-listener, plaintext/dynamic-limit/slot-recovery, pre-TLS rejection, and idle-connection tests passed (4/4). Merge-base diff check passed. Full suite and macOS fallback were not run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It changes core connection-acceptance behavior (including SSL-path early rejection) and public API/ABI surface, warranting final human review despite targeted tests and documentation.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants