67 - reworked the way an HTTP-Client is aquired when calling a remote…

[tomsontom]

… method

[tomsontom]

Review

Overview

Reworks how the generated JDK HTTP client obtains its java.net.http.HttpClient. Previously the client cached a single Supplier<HttpClient> for its whole lifetime and exposed close() (the client itself was AutoCloseable). Now it introduces an HttpClientProvider interface (acquire()/release()), and each generated service operation acquires a client at the start of the call via try (var $clientSupplier = this.client.httpClientSupplier()) { ... } and releases it when the try block exits — this is presumably aimed at supporting pooled/Apache-style clients where a connection should be acquired/returned per request rather than held forever.

The hand-written change is confined to packages/cli/src/java-rest-client-jdk/client.ts and service.ts; the other 23 files are regenerated/sample output that consistently mirrors the template change. Verified mvn clean compile succeeds for java-test/java-client (182 files) on this branch, and npm run test --workspace packages/cli shows no new failures versus main (the only failures are pre-existing ECONNREFUSED localhost:3000 cases in the TS fetch-client tests, unrelated to this Java-client change).

Issues

• Likely perf regression in the default path. DefaultHttpClientProvider.acquire() returns HttpClient.newBuilder().build() — a brand-new HttpClient — and release() calls client.close(). Since httpClientSupplier() is now invoked once per service-method call (wrapped per-operation in the generated try (...)), the default provider creates and tears down a new HttpClient (and its internal selector/executor) for every single request, whereas before a single HttpClient was created once and reused for the lifetime of the client (JDK's HttpClient is thread-safe and meant to be shared/reused for connection keep-alive). This looks like it defeats connection pooling/keep-alive for anyone using the default provider and adds non-trivial per-call overhead. If pooling is the goal, the default provider should probably hand out a shared/cached instance from acquire() with a no-op (or ref-counted) release(), leaving real acquire/release semantics to custom providers backed by an actual pool.
• Typo: HttpCLientSupplier (capital L) in both client.ts's generated source and the hand-written JDKSpecSamplesClient.java. Not a functional bug, but worth fixing before this name shows up in public generated APIs.
• Minor: HttpClient.newBuilder().build() in DefaultHttpClientProvider dropped the previous .version(HttpClient.Version.HTTP_1_1) pin. If that pin was deliberate (e.g. to work around an HTTP/2 issue), this silently reverts to JDK defaults (HTTP/2 with HTTP/1.1 fallback) — worth confirming this is intentional.

Things that look right

• Removing AutoCloseable/close() from the client class is consistent with moving lifecycle management to the per-request supplier; the test files were correctly updated to drop the now-invalid close() calls.
• The httpClient(HttpClient) builder overload correctly wraps the user-supplied instance in a no-op-release provider, preserving "I manage this client myself" semantics.
• All call sites generating this.client.httpClient() were consistently migrated to the new $clientSupplier.get() pattern — no stragglers found.

[tomsontom]

Follow-up on my earlier review: the default-provider behavior (new HttpClient per call via DefaultHttpClientProvider) is intentional, not an oversight. Per the author: since the client has no explicit lifecycle/close hook anymore, creating and destroying a fresh HttpClient per call is the defensive choice — it avoids ever leaking a long-lived client that nobody is responsible for closing. Anyone who wants connection reuse/pooling is free to supply a custom HttpClientProvider that hands back a cached instance with a no-op release().

Retracting that point as a concern — flagging here so it doesn't get re-raised by other reviewers.