Fix ClientFactory leak in KubernetesEndpointGroup watches - #6909
Open
arimu1 wants to merge 2 commits into
Open
Conversation
Derived ArmeriaHttpClient instances (used for watches/tags) each created a new AtomicBoolean and ArmeriaWebSocketClient, so WebSocket ClientFactory objects built during KubernetesEndpointGroup watches were never closed when the root KubernetesClient was closed. Share closed state, WebClient, and WebSocket transport with the root client, matching fabric8 JDK/Jetty adapters. Fixes line#6805
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Kubernetes client now shares WebSocket resources and close state between root and derived HTTP clients. A lifecycle test verifies shared instances, lazy WebSocket factory creation, and shutdown propagation. ChangesKubernetes client lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ArmeriaHttpClientBuilder
participant DerivedArmeriaHttpClient
participant ArmeriaWebSocketClient
participant RootArmeriaHttpClient
ArmeriaHttpClientBuilder->>DerivedArmeriaHttpClient: share WebClient, WebSocket client, and close state
DerivedArmeriaHttpClient->>ArmeriaWebSocketClient: open WebSocket
ArmeriaWebSocketClient-->>DerivedArmeriaHttpClient: create or return WebSocket client
RootArmeriaHttpClient->>DerivedArmeriaHttpClient: propagate close state
RootArmeriaHttpClient->>ArmeriaWebSocketClient: close shared WebSocket factory
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Tick the box to add this pull request to the merge queue (same as
|
jrhee17
approved these changes
Aug 10, 2026
Comment on lines
+58
to
+61
| // Share close state, HTTP client, and WebSocket transport with the root client so that | ||
| // ClientFactory instances created for watches are released when the root is closed. | ||
| // Matches fabric8 JDK/Jetty clients; without this, each derived client builds its own | ||
| // WebSocket ClientFactory that is never closed (LEAK / #6805). |
Contributor
There was a problem hiding this comment.
Suggested change
| // Share close state, HTTP client, and WebSocket transport with the root client so that | |
| // ClientFactory instances created for watches are released when the root is closed. | |
| // Matches fabric8 JDK/Jetty clients; without this, each derived client builds its own | |
| // WebSocket ClientFactory that is never closed (LEAK / #6805). |
Optional) I think the intention of this code is already clear from the constructor javadocs already
Comment on lines
+139
to
+142
| /** | ||
| * Returns the {@link ClientFactory} used by the underlying {@link WebSocketClient}, or | ||
| * {@code null} if the WebSocket client has not been created yet. | ||
| */ |
Contributor
There was a problem hiding this comment.
Suggested change
| /** | |
| * Returns the {@link ClientFactory} used by the underlying {@link WebSocketClient}, or | |
| * {@code null} if the WebSocket client has not been created yet. | |
| */ |
Optional) no need for javadocs for obvious methods
minwoox
approved these changes
Aug 10, 2026
| * {@code null} if the WebSocket client has not been created yet. | ||
| */ | ||
| @Nullable | ||
| ClientFactory clientFactoryOrNull() { |
Contributor
ikhoon
reviewed
Aug 11, 2026
| if (webSocketClient == null) { | ||
| return null; | ||
| } | ||
| return webSocketClient.options().factory(); |
Contributor
There was a problem hiding this comment.
If this is only for testing, the amount of code should be minimized. Should we add a getter for this.webSocketClient instead?
Replace clientFactoryOrNull helper with a minimal package-private getter for the underlying WebSocketClient field so tests can inspect factory lifecycle without extra factory-wrapping code.
Author
|
@ikhoon Thanks — replaced Tip: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
KubernetesEndpointGroupwatches open WebSockets through fabric8's derivedHttpClients (e.g. tagged withRequestConfig). Armeria's derivedArmeriaHttpClientdid not share the close flag or WebSocket transport withthe root client (unlike fabric8's JDK/Jetty adapters). Each derived client
built its own WebSocket
ClientFactory, which was never closed when the rootKubernetesClientwas closed, producing:Stack traces point at
ArmeriaWebSocketClient.webSocketClient()viaKubernetesEndpointGroup.doWatchService(#6805).Modifications:
AtomicBoolean closed,WebClient, andArmeriaWebSocketClientwhenbuilding a derived
ArmeriaHttpClientfromArmeriaHttpClientBuilder.ClientFactorylifecycle.shared WebSocket
ClientFactoryis closed.Result:
created for watches on derived clients.