fix: Prevent OkHttp idle threads from blocking JVM shutdown#1268
Conversation
|
Hi @svetanis, thank you for your contribution! We appreciate you taking the time to submit this pull request. Currently this PR is under review by our team, we will keep you posted if any additional information is required. thank you. |
|
@anFatum, Could you please review this PR. |
f92a4ab to
b95dd45
Compare
|
Hi @hemasekhar-p, Thanks for the update! Just a quick heads up: I went ahead and rebased the branch onto the latest Looking forward to hearing back from the team! |
krwc
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Left some comments requiring changes, but no major issues.
| @@ -0,0 +1,62 @@ | |||
| /* | |||
| * Copyright 2025 Google LLC | |||
| @@ -0,0 +1,29 @@ | |||
| package com.google.adk.utils; | |||
There was a problem hiding this comment.
Please add copyright headers to this file.
| .connectTimeout(Duration.ofMinutes(5)) | ||
| .readTimeout(Duration.ofMinutes(5)) | ||
| .writeTimeout(Duration.ofMinutes(5)) |
There was a problem hiding this comment.
OkHttp's default timeouts are 10s. Any reason we decided to set them to 5mins now? If not, let's not change them to maintain backwards compat and let the callers decide.
| import okhttp3.OkHttpClient; | ||
|
|
||
| /** Utility class for common HTTP client configuration across the ADK. */ | ||
| public final class HttpUtils { |
There was a problem hiding this comment.
While we can't make this class truly package private due to its use in multiple packages, I still think we should move it to somewhere else, indicating its not truly intended to be a part of public API of Java ADK.
How about we move it into core/src/main/java/com/google/adk/internal/http?
| import okhttp3.OkHttpClient; | ||
|
|
||
| /** Utility class for common HTTP client configuration across the ADK. */ | ||
| public final class HttpUtils { |
…hutdown This fixes an issue where the JVM hangs waiting for background networking threads to timeout after completing an execution. Replaced default OkHttpClient builders with a new HttpUtils.createSharedHttpClient factory that injects a custom thread factory setting daemon=true.
b95dd45 to
9b046b6
Compare
|
Thanks for the review! I've addressed all your comments:
Rebased onto latest |
|
FYI: we actually had to roll it back due to some issue with one of the internal systems using it. Will investigate this and try to forward fix. |
|
@krwc , Thanks for the heads-up. Sorry to hear about the issue. One area that might be worth investigating is the timeout configuration. The final version of the PR removed the explicit timeouts from the shared Happy to help with the forward fix. A couple of options:
The daemon thread change itself should be safe to keep. Let me know how you'd like to proceed. |
|
@krwc , Following up on my earlier comment. I dug into this a bit more and I think I understand what's happening. It changes both where the problem is and what I'd suggest for the fix. The change silently dropped Gemini/Vertex transport timeouts from infinite to 10s, on the Gemini path. In One correction to my earlier comment: I'd floated restoring ~5-minute defaults, but the right value is infinite, for exact parity with the SDK. On this path OkHttp's 10s default isn't the backwards-compatible value; the SDK's infinite is. Proposed forward fix (keeps the JVM-shutdown fix from #1267/#1268):
How would you like to proceed? I can open a new issue and a PR, just a PR referencing #1267/#1268, or hold off while you handle it internally. Please let me know what's easiest on your end. |
|
We will handle this internally, I'm working on a relevant change in the background. Thanks! |
Prevent OkHttp idle threads from blocking JVM shutdown
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
The ADK framework does not cleanly terminate the JVM upon process completion when making chat completions requests because
ChatCompletionsHttpClientand the internalGeminiAPI client construct defaultOkHttpClientinstances. These produce non-daemon threads with a keep-alive timeout, keeping the JVM alive.Solution:
Created
HttpUtils.createSharedHttpClient()utility method that returns anOkHttpClientusing a custom dispatcher with a daemon thread factory and 5-minute timeouts. RefactoredChatCompletionsHttpClient,ApiClient, andGeminito invoke this utility and maintain their own shared daemon-thread client pools.Testing Plan
Unit Tests:
All existing core tests pass successfully. (A new test for HttpUtils can be added to verify the ThreadFactory creates daemon threads).
Manual End-to-End (E2E) Tests:
Wired the Google native chat completion client into the external
model-prismproject (PR #1199). Ran the integration through all the demo tests covering all main ADK features. Explicitly tested the native Gemini provider (via thegoogle_search_agenttests) as well as third-party OpenAI-compatible providers. After the final agent response is logged, the Java process successfully exits cleanly with code 0 across all model types instead of hanging while waiting for background threads to timeout.Checklist
Additional context
This fixes critical thread leaks for users running short-lived or background ADK agents.