feat(java): opt-in enable-closeable-client for ownership-aware AutoCloseable root clients - #17771
devin-ai-integration[bot] wants to merge 6 commits into
Conversation
…re AutoCloseable root clients Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: bot_apk <apk@cognition.ai>
There was a problem hiding this comment.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| .beginControlFlow("if (!this.clientOptions.ownsHttpClient())") | ||
| .addStatement("return") | ||
| .endControlFlow() | ||
| .addStatement("this.clientOptions.httpClient().dispatcher().executorService().shutdown()") | ||
| .addStatement("this.clientOptions.httpClient().connectionPool().evictAll()") |
There was a problem hiding this comment.
🔴 Auth transport survives root closure
With inferred or endpoint-scoped auth, close() releases only the root clientOptions transport. The generated auth provider retains a separate SDK-created authClient, so its dispatcher can keep the JVM alive.
Learn more
Several auth configurations create an internal client to call the token endpoint. The inferred-auth path builds that client from a fresh auth client options builder. Endpoint-security OAuth and inferred-auth providers similarly retain independently built auth clients. Each fresh options builder creates and owns another OkHttp dispatcher and connection pool. The root client stores only its main ClientOptions, so the new close() cannot reach those internal transports. Once token fetching uses an internal dispatcher, closing the root can still leave its threads alive.
Example: An endpoint-security SDK builds a root client with OAuth credentials. Its first authenticated request uses the retained token-fetching authClient. Closing the root shuts down only the API transport; the token transport remains active.
Recommended fix: Track every SDK-created auth transport in the root client's lifecycle. Close each owned dispatcher and connection pool, while deduplicating shared resources and preserving caller-owned transports.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed: the inferred-auth and endpoint-security OAuth paths build a separate ClientOptions (fresh dispatcher/pool) for the token-fetching AuthClient, and the root's close() can't reach it. The staged OAuth client-credentials path is covered now (it shares baseOptions' client — see the other thread), which is the common OAuth case; Deepgram's SDK uses API-key auth and is unaffected. Reaching the other auth transports means the root client must retain them, which is a larger change than this opt-in flag — documented as a known limitation in the PR description and left for a follow-up rather than expanding scope here.
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
…or OAuth build path Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
…t in changelog Co-Authored-By: bot_apk <apk@cognition.ai>
…add local_files seed variant Co-Authored-By: bot_apk <apk@cognition.ai>
Description
Linear ticket: Refs Deepgram request (hand-written patch: deepgram/deepgram-java-sdk#108)
Adds an opt-in
enable-closeable-clientconfig flag to the Java SDK generator. When enabled, the generated sync and async root clients implementAutoCloseable, andClientOptionsrecords whether the SDK created the underlyingOkHttpClientsoclose()only releases SDK-owned resources.Problem
The generated root clients expose no way to release the
OkHttpClientthe SDK creates. In short-lived JVMs (especially after WebSocket use) OkHttp's dispatcherExecutorServiceand connection pool hold non-daemon threads, so the JVM does not exit promptly. Deepgram currently carries a hand-written patch for this; this PR lets the generator emit it natively so they can delete the patch.Why ownership tracking is required (
newBuilder()shares resources)ClientOptions.Builder.build()derives the client the SDK actually uses viathis.httpClient != null ? this.httpClient.newBuilder() : new OkHttpClient.Builder().Verified in OkHttp source (
OkHttpClient.kt,Builder(okHttpClient: OkHttpClient)):newBuilder()copiesthis.dispatcher = okHttpClient.dispatcherandthis.connectionPool = okHttpClient.connectionPool, i.e. the derived client shares the caller'sDispatcherandConnectionPool. An unconditionalclose()would therefore shut down the caller's dispatcher and evict their pool. Henceclose()must be conditional on ownership.Design: ownership on
ClientOptions, not the builderDeepgram's patch tracks ownership on the root client builder and passes it through a package-private constructor. That leaves a gap: the public
new XClient(clientOptions)constructor always reports not-owned, so direct construction (and hand-written subclasses) get a no-opclose().Here ownership is a field on
ClientOptions, computed inClientOptions.Builder.build()beforethis.httpClientis replaced by the derived client:Root clients (sync + async, emitted from the shared
AbstractRootClientGenerator):close()narrowsAutoCloseable.close()to declare no checked exception (try-with-resources without a catch), and is idempotent (shutdown()on a shut-down executor andevictAll()on an empty pool are no-ops). Any code path that ends in aClientOptions— builder, direct constructor, subclasses — gets correct behaviour.Ownership follows the client through the builder:
Builder.ownsHttpClientstartstrue,httpClient(OkHttpClient)sets itfalse, andBuilder.from(existing)copiesexisting.ownsHttpClient(). This matters for the generated OAuth client-credentials build path, which doesBuilder.from(baseOptions).addHeader(..., tokenSupplier).build()—baseOptionsis SDK-created, and the derived options share its dispatcher/pool vianewBuilder(), so the resulting root client must still own (and close) them. Afrom(callerOwnedOptions)derivation stays caller-owned.Known limitation (stated in the changelog too): the inferred-auth and endpoint-security OAuth paths (
setAuthentication/AuthProviderInfobranches inAbstractRootClientGenerator) build a separateClientOptions— hence a separateOkHttpClient— for their token-fetchingAuthClient, whose dispatcher is not reachable from the root client'sclose(). Fixing it means either sharing one transport between the root and auth options (they're built in different orders insidebuildClientOptions(), and ownership would have to be handed across packages without going through the publichttpClient(...)setter) or making the root client retain the auth transports — both change how auth transports are constructed, so it is deliberately left out of this opt-in PR. Deepgram's SDK uses API-key auth and is unaffected.Why opt-in
Adding
implements AutoCloseable, a newClientOptionsconstructor parameter, and a new getter is a public-surface change. Per the generator breaking-changes policy the new behaviour is gated; defaultfalsekeeps generated output byte-identical to 4.19.3 — none of the new field/getter/constructor param/interface/method is emitted when the flag is off.Scope boundary
Only the dispatcher executor and connection pool are released. WebSocket reconnect scheduler threads are intentionally not touched — Deepgram tracks that separately in deepgram/deepgram-java-sdk#107.
Changes Made
JavaSdkCustomConfig: new@JsonProperty("enable-closeable-client") enableCloseableClient()(defaultfalse), same@Value.Default+ Javadoc style asallowUserAgentAppInfo/respect-optional-request-body.ClientOptionsGenerator: gatedownsHttpClientfield, constructor param/assignment,ownsHttpClient()getter, gatedBuilder.ownsHttpClientstate (cleared byhttpClient(...), copied byfrom(...)), andboolean ownsHttpClient = this.httpClient == null || this.ownsHttpClient;at the top ofbuild().JavaSdkDownloadFilesCustomConfig+Cli.runInDownloadFilesModeHook: the download-files (local-file-system) path rebuildsJavaSdkCustomConfigfrom an explicit field allowlist, so the flag is declared there too and forwarded via.enableCloseableClient(...). Without this the flag was silently dropped in local generation (the mode Deepgram uses).AbstractRootClientGenerator: gatedaddSuperinterface(AutoCloseable.class)+buildCloseMethod()applied to both sync and async root clients.changes/unreleased/feat-closeable-client.yml(type: feat;versions.ymlis generated by release automation and not hand-edited in this repo).closeable-clientvariants (enable-closeable-client: true) forimdbandoauth-client-credentialsinseed/java-sdk/seed.yml+ snapshots; the OAuth one exercises theBuilder.from(baseOptions)ownership carry-over. A thirdimdbvariant,closeable-client-local-files(outputMode: local_files), guards the download-files allowlist.Testing
CloseableClientTestcompiles the exact emittedclose()against a realOkHttpClientand asserts: SDK-owned client's dispatcher executor is shut down after try-with-resources; caller-owned client (vianewBuilder()-derived client, assertingdispatcher()/connectionPool()areisSameAsthe caller's) is left running;close()is idempotent;close()declares no checked exceptions.cd generators/java && ./gradlew spotlessApply test— BUILD SUCCESSFUL (sdk: 75 tests, 0 failures).pnpm seed test --generator java-sdk --fixture imdb --fixture oauth-client-credentials --skip-scripts --local— 10/10 variants pass (incl.closeable-client-local-files, whoseSeedApiClient/AsyncSeedApiClient/core/ClientOptions.javacontain theAutoCloseable/ownsHttpClientoutput).imdbvariants and 2 pre-existingoauth-client-credentialsvariants (none set the flag) were regenerated with this generator build;git diffon their sources is empty (0 lines; only the seed-runnermetadata.jsontimestamps churned and were restored).generators/java/sdk/versions.ymltop entry is 4.19.3 andchanges/unreleased/contained no other entries, so the checked-in snapshots are 4.19.3 output.seed/java-sdk/imdb/closeable-client) compiles with./gradlew compileJava.Link to Devin session: https://app.devin.ai/sessions/0da4d7df52254236b57eede700e42a2e
Open in Devin Desktop: https://app.devin.ai/desktop/session/0da4d7df52254236b57eede700e42a2e?variant=devin