Skip to content

feat(java): opt-in enable-closeable-client for ownership-aware AutoCloseable root clients - #17771

Open
devin-ai-integration[bot] wants to merge 6 commits into
mainfrom
devin/1789652480-java-closeable-client
Open

devin-ai-integration[bot] wants to merge 6 commits into
mainfrom
devin/1789652480-java-closeable-client

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Description

Linear ticket: Refs Deepgram request (hand-written patch: deepgram/deepgram-java-sdk#108)

Adds an opt-in enable-closeable-client config flag to the Java SDK generator. When enabled, the generated sync and async root clients implement AutoCloseable, and ClientOptions records whether the SDK created the underlying OkHttpClient so close() only releases SDK-owned resources.

Problem

The generated root clients expose no way to release the OkHttpClient the SDK creates. In short-lived JVMs (especially after WebSocket use) OkHttp's dispatcher ExecutorService and 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 via
this.httpClient != null ? this.httpClient.newBuilder() : new OkHttpClient.Builder().
Verified in OkHttp source (OkHttpClient.kt, Builder(okHttpClient: OkHttpClient)): newBuilder() copies this.dispatcher = okHttpClient.dispatcher and this.connectionPool = okHttpClient.connectionPool, i.e. the derived client shares the caller's Dispatcher and ConnectionPool. An unconditional close() would therefore shut down the caller's dispatcher and evict their pool. Hence close() must be conditional on ownership.

Design: ownership on ClientOptions, not the builder

Deepgram'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-op close().

Here ownership is a field on ClientOptions, computed in ClientOptions.Builder.build() before this.httpClient is replaced by the derived client:

public ClientOptions build() {
    boolean ownsHttpClient = this.httpClient == null;
    OkHttpClient.Builder httpClientBuilder =
            this.httpClient != null ? this.httpClient.newBuilder() : new OkHttpClient.Builder();
    ...
    return new ClientOptions(environment, headers, headerSuppliers, httpClient, ownsHttpClient, ...);
}

public boolean ownsHttpClient() { return this.ownsHttpClient; }

Root clients (sync + async, emitted from the shared AbstractRootClientGenerator):

public class SeedApiClient implements AutoCloseable {
    ...
    /**
     * Releases resources owned by an SDK-created HTTP client. A client supplied through
     * {@code httpClient(OkHttpClient)} remains owned by the caller and is left untouched.
     */
    @Override
    public void close() {
        if (!this.clientOptions.ownsHttpClient()) {
            return;
        }
        this.clientOptions.httpClient().dispatcher().executorService().shutdown();
        this.clientOptions.httpClient().connectionPool().evictAll();
    }
}

close() narrows AutoCloseable.close() to declare no checked exception (try-with-resources without a catch), and is idempotent (shutdown() on a shut-down executor and evictAll() on an empty pool are no-ops). Any code path that ends in a ClientOptions — builder, direct constructor, subclasses — gets correct behaviour.

Ownership follows the client through the builder: Builder.ownsHttpClient starts true, httpClient(OkHttpClient) sets it false, and Builder.from(existing) copies existing.ownsHttpClient(). This matters for the generated OAuth client-credentials build path, which does Builder.from(baseOptions).addHeader(..., tokenSupplier).build()baseOptions is SDK-created, and the derived options share its dispatcher/pool via newBuilder(), so the resulting root client must still own (and close) them. A from(callerOwnedOptions) derivation stays caller-owned.

Known limitation (stated in the changelog too): the inferred-auth and endpoint-security OAuth paths (setAuthentication / AuthProviderInfo branches in AbstractRootClientGenerator) build a separate ClientOptions — hence a separate OkHttpClient — for their token-fetching AuthClient, whose dispatcher is not reachable from the root client's close(). Fixing it means either sharing one transport between the root and auth options (they're built in different orders inside buildClientOptions(), and ownership would have to be handed across packages without going through the public httpClient(...) 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 new ClientOptions constructor parameter, and a new getter is a public-surface change. Per the generator breaking-changes policy the new behaviour is gated; default false keeps 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() (default false), same @Value.Default + Javadoc style as allowUserAgentAppInfo / respect-optional-request-body.
  • ClientOptionsGenerator: gated ownsHttpClient field, constructor param/assignment, ownsHttpClient() getter, gated Builder.ownsHttpClient state (cleared by httpClient(...), copied by from(...)), and boolean ownsHttpClient = this.httpClient == null || this.ownsHttpClient; at the top of build().
  • JavaSdkDownloadFilesCustomConfig + Cli.runInDownloadFilesModeHook: the download-files (local-file-system) path rebuilds JavaSdkCustomConfig from 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: gated addSuperinterface(AutoCloseable.class) + buildCloseMethod() applied to both sync and async root clients.
  • changes/unreleased/feat-closeable-client.yml (type: feat; versions.yml is generated by release automation and not hand-edited in this repo).
  • Seed: new closeable-client variants (enable-closeable-client: true) for imdb and oauth-client-credentials in seed/java-sdk/seed.yml + snapshots; the OAuth one exercises the Builder.from(baseOptions) ownership carry-over. A third imdb variant, closeable-client-local-files (outputMode: local_files), guards the download-files allowlist.
  • Updated README.md generator (N/A)

Testing

  • Unit tests added/updated — CloseableClientTest compiles the exact emitted close() against a real OkHttpClient and asserts: SDK-owned client's dispatcher executor is shut down after try-with-resources; caller-owned client (via newBuilder()-derived client, asserting dispatcher()/connectionPool() are isSameAs the 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, whose SeedApiClient/AsyncSeedApiClient/core/ClientOptions.java contain the AutoCloseable/ownsHttpClient output).
  • Flag-off output unchanged vs 4.19.3: the 5 pre-existing imdb variants and 2 pre-existing oauth-client-credentials variants (none set the flag) were regenerated with this generator build; git diff on their sources is empty (0 lines; only the seed-runner metadata.json timestamps churned and were restored). generators/java/sdk/versions.yml top entry is 4.19.3 and changes/unreleased/ contained no other entries, so the checked-in snapshots are 4.19.3 output.
  • Flag-on snapshot (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


Devin Review

…re AutoCloseable root clients

Co-Authored-By: bot_apk <apk@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

nitpickybot[bot]

This comment was marked as resolved.

Co-Authored-By: bot_apk <apk@cognition.ai>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment on lines +246 to +250
.beginControlFlow("if (!this.clientOptions.ownsHttpClient())")
.addStatement("return")
.endControlFlow()
.addStatement("this.clientOptions.httpClient().dispatcher().executorService().shutdown()")
.addStatement("this.clientOptions.httpClient().connectionPool().evictAll()")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

SDK Generation Benchmark Results

Comparing PR branch against median of 5 nightly run(s) on main (latest: 2026-09-17T04:06:38Z).

Full benchmark table (click to expand)
Generator Spec main (generator) main (E2E) PR (generator) Delta
java-sdk square 221s (n=5) 272s (n=5) 227s +6s (+2.7%)

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 fern generate). main (E2E): full customer-observable time including build/test scripts (nightly baseline, informational). Delta is computed against generator-only baseline.
⚠️ = generation exited with a non-zero exit code (timing may not reflect a successful run).
Baseline from nightly runs on main (latest: 2026-09-17T04:06:38Z). Trigger benchmark-baseline to refresh.
Last updated: 2026-09-17 14:42 UTC

devin-ai-integration Bot and others added 4 commits September 17, 2026 13:51
…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>
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.

0 participants