fix: preserve shared resources across derived clients - #851
fix: preserve shared resources across derived clients#851abhinavkr26104 wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 183fd87ec8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| httpClientResource = clientOptions.resources.httpClient | ||
| httpRequestAuthenticatorResource = clientOptions.resources.httpRequestAuthenticator | ||
| workloadIdentityAuthResource = clientOptions.resources.workloadIdentityAuth | ||
| streamHandlerExecutorResource = clientOptions.resources.streamHandlerExecutor | ||
| sleeperResource = clientOptions.resources.sleeper |
There was a problem hiding this comment.
When code keeps only the ClientOptions.Builder returned by toBuilder() and lets the source ClientOptions become unreachable before calling build(), these copied resource handles are not counted as references. The new ClientOptions cleaner can therefore release and mark the shared ClientOptionsResources closed while the builder still intends to reuse them, and the later build() hits retain() on a closed resource instead of producing the derived options. Retain the resources for the builder's lifetime or keep the source options strongly reachable until build() transfers ownership.
Useful? React with 👍 / 👎.
| httpRequestAuthenticator?.release() | ||
| workloadIdentityAuth?.release() | ||
| httpClient.release() | ||
| streamHandlerExecutor.release() | ||
| sleeper.release() |
There was a problem hiding this comment.
When a provider HttpRequestAuthenticator.close() throws while closing the last owning options/client, this first release propagates immediately, so the HTTP client, stream executor, and sleeper are never released and later close() calls are no-ops through ClientOptionsCloseAction. The previous AuthenticatingHttpClient.close() path still closed the delegate after authenticator failure, so please keep releasing the remaining resources with suppressed exceptions in this failure path.
Useful? React with 👍 / 👎.
| if (checkJacksonVersionCompatibility) { | ||
| checkJacksonVersionCompatibility() | ||
| } | ||
| closeWhenPhantomReachable(this, closeAction) |
There was a problem hiding this comment.
When callers keep only the CompletableFuture returned by an async request and let the client/service become unreachable, this cleaner can run as soon as the ClientOptions is phantom-reachable even though the returned future is still using the RetryingHttpClient chain. That closes the underlying HTTP client/executor/sleeper out from under an in-flight request; the prior phantom cleanup was attached to the HTTP wrapper itself, so the resources stayed reachable while the async pipeline still referenced that wrapper.
Useful? React with 👍 / 👎.
|
Addressed the P2 resource-lifetime feedback in commit Changes:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24dc9ae37a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| // Async request futures retain the HTTP client chain, not this options object. Observe the | ||
| // chain so phantom cleanup cannot close resources while an in-flight request still uses it. | ||
| closeWhenPhantomReachable(httpClient, closeAction) |
There was a problem hiding this comment.
For async uploads with InputStream multipart parts, HttpRequestBody.repeatable() is false and RetryingHttpClient.executeAsync() returns the delegate responseFuture directly instead of a future that captures this RetryingHttpClient; registering the cleaner on this wrapper can therefore still release the shared HTTP client/sleeper while the only remaining reference is the in-flight delegate future. Fresh evidence for re-raising the earlier in-flight concern is this non-retryable branch, which bypasses the new reachability chain entirely.
Useful? React with 👍 / 👎.
| ClientOptions.builder() | ||
| .httpClient(httpClient) | ||
| .sleeper(sleeper) | ||
| .streamHandlerExecutor(executor) | ||
| .build() |
There was a problem hiding this comment.
When this new test helper runs, ClientOptions.builder().build() still enforces that at least one credential source is configured, so the test throws IllegalStateException before it ever starts the pending async request or reaches the GC assertions. Add an API key/admin key or a test authenticator here so the new regression test can actually exercise the resource-lifetime behavior instead of failing unconditionally.
Useful? React with 👍 / 👎.
| clientOptionsWithUserAgent.close() | ||
| if (clientOptionsWithUserAgent !== clientOptions) clientOptions.close() |
There was a problem hiding this comment.
When async() has been accessed on a sync client, the lazy child constructs its own clientOptionsWithUserAgent and retains the same ClientOptionsResources; this close path only releases the parent UA options and the original options, so the refcount stays above zero and explicit client.close() leaves the HTTP client/executor/sleeper open as long as the parent still holds that lazy child. Close initialized child views as well, or avoid giving them a separately retained options instance.
Useful? React with 👍 / 👎.
|
|
||
| fun workloadIdentity(workloadIdentity: WorkloadIdentity?) = apply { | ||
| this.workloadIdentity = workloadIdentity | ||
| this.workloadIdentityAuthResource = null |
There was a problem hiding this comment.
When options created with workloadIdentity(...) are copied and the modifier calls workloadIdentity(null) without replacing the credential, from() has already copied the existing WorkloadIdentityCredential, so effectiveCredential() still reuses the same WorkloadIdentityAuth. Clearing workloadIdentityAuthResource here makes the derived options create an independent close resource for that same auth object, so closing the derived options can close the provider while the original options still rely on it, and closing both can close it twice.
Useful? React with 👍 / 👎.
1 participant
Summary
ClientOptions.toBuilder()/withOptions()viewsTesting
git diff --check./gradlew :openai-java-core:test --tests com.openai.core.ClientOptionsTest --no-daemon --console=plain(timed out without diagnostics in the local environment)./gradlew :openai-java-core:compileKotlin --no-daemon --max-workers=1 --offline --console=plain(timed out without diagnostics in the local environment)Fixes #850