Skip to content

[rpc] Avoid retaining failed connections in the connection pool - #4884

Open
beryllw wants to merge 4 commits into
apache:mainfrom
beryllw:codex/fix-netty-connection-registration
Open

beryllw wants to merge 4 commits into
apache:mainfrom
beryllw:codex/fix-netty-connection-registration

Conversation

@beryllw

@beryllw beryllw commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Closes #4904.

An immediate connection failure on a Netty event loop can invoke the removal callback inside connections.computeIfAbsent(). The recursive removal fails, leaving a disconnected connection cached and preventing subsequent requests from reconnecting.

Brief change log

  • Defer connection removal to the client's event-loop group to avoid recursive map updates. Keep identity-checked removal so delayed cleanup cannot remove a replacement connection.
  • Preserve connection initialization. An immediate retry may still fail before cleanup runs; subsequent retries can create a new connection.

Tests

  • Add one regression for immediate DNS failure, eventual removal, and successful reconnection with the same client. It fails without the fix.
  • Targeted test: all 14 tests in NettyClientTest and ServerConnectionTest passed, with one endpoint test rerun after a local port conflict. Formatting, Checkstyle and license checks passed.
  • An earlier full-reactor attempt hit metadata-related integration failures and forked-JVM exits in other modules.

API and Format

No public API, RPC protocol or storage format changes.

Documentation

Not required.

Generative AI disclosure:

  • Yes — OpenAI Codex.

beryllw and others added 3 commits October 26, 2026 18:03
Register connections before starting an idempotent connect operation so
immediate connection failures can remove the cached entry safely.

Add regressions for immediate address resolution failure followed by
reconnection and for concurrent requests sharing one connection.

Co-Authored-By: Codex <noreply@openai.com>
AI-Model: gpt-6
AI-Contributed/Feature: 64/64
AI-Contributed/UT: 114/114
Remove the test-only auto-connect constructor and start connections
explicitly in existing tests. Reuse the single-loop client fixture,
simplify the resolver stub, and trim redundant assertions and comments.

Co-Authored-By: Codex <noreply@openai.com>
AI-Model: gpt-6
AI-Contributed/Feature: 14/14
AI-Contributed/UT: 38/38
Queue connection eviction to avoid re-entering computeIfAbsent on an
immediate connection failure. Keep conditional removal and the recovery
regression, without changing ServerConnection initialization.

Co-Authored-By: Codex <noreply@openai.com>
AI-Model: gpt-6
AI-Contributed/Feature: 51/51
AI-Contributed/UT: 42/42
@beryllw beryllw changed the title [rpc] Avoid caching connections closed during construction [rpc] Avoid retaining failed connections in the connection pool Oct 27, 2026
@beryllw
beryllw requested a review from loserwang1024 October 27, 2026 05:57
@beryllw
beryllw marked this pull request as ready for review October 27, 2026 05:57
Run the immediate DNS failure regression with one and four network threads.
Keep connection establishment on the caller's event loop and wait for
asynchronous cleanup before verifying recovery with the same client.

Co-Authored-By: Codex <noreply@openai.com>
AI-Model: gpt-6
AI-Contributed/Feature: 0/0
AI-Contributed/UT: 18/18
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.

[rpc] Immediate connection failure can leave a disconnected connection cached

1 participant