Skip to content

61 - added possibility to provide http-client and added close API#62

Merged
tomsontom merged 2 commits into
mainfrom
61-allow-users-to-better-control-how-many-httpclients-are-used
Jun 19, 2026
Merged

61 - added possibility to provide http-client and added close API#62
tomsontom merged 2 commits into
mainfrom
61-allow-users-to-better-control-how-many-httpclients-are-used

Conversation

@tomsontom

Copy link
Copy Markdown
Member

No description provided.

@tomsontom tomsontom linked an issue Jun 18, 2026 that may be closed by this pull request

@tomsontom tomsontom left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Review

Bug: Dead HttpClient in build()

Most critical issue. In both the template (client.ts) and the fixture (JDKSpecSamplesClient.java):

if (httpClientSupplier == null) {
    var client = HttpClient.newBuilder().build();  // created and immediately discarded
    return new JDKSpecSamplesClient(baseURI, new InternalClientSupplier(), contentType);
}

var client is never used — InternalClientSupplier creates its own HttpClient internally. This is a resource leak (an HttpClient with an open thread pool is abandoned to the GC). The line should simply be removed.

Design: Template hardcodes APPLICATION_JSON as default

The original template used ${toEnumLiteral(contentEncodings[0])} to respect the configured content encoding order. The new code hardcodes APPLICATION_JSON:

var contentType = contentTypeEncoding == null ? ContentTypeEncoding.APPLICATION_JSON : contentTypeEncoding;

Should remain ContentTypeEncoding.${toEnumLiteral(contentEncodings[0])} so the default honours generator config.

Minor: close() doesn't handle user-provided AutoCloseable suppliers

public void close() {
    if (this.httpClientSupplier instanceof InternalClientSupplier) { ... }
}

If a caller passes an httpClientSupplier that is itself AutoCloseable (e.g., a pool factory), it will silently not be closed. Consider also checking instanceof AutoCloseable:

public void close() {
    if (this.httpClientSupplier instanceof InternalClientSupplier) {
        this.httpClientSupplier.get().close();
    } else if (this.httpClientSupplier instanceof AutoCloseable ac) {
        try { ac.close(); } catch (Exception e) { throw new IllegalStateException(e); }
    }
}

Positives

  • AutoCloseable + Supplier<HttpClient> is the right abstraction
  • Test teardown simplified to .close() — much cleaner
  • httpClientSupplier() builder method opens the door to connection-pool patterns
  • InternalClientSupplier encapsulates the internally-managed client cleanly

Must fix: the dead var client line in build(). The template default encoding regression should also be addressed. The AutoCloseable supplier handling is optional but worth considering.

- removed dead code
- fixed hard coding of content-type
- allow diposing of the closeable suppliers
@tomsontom tomsontom merged commit 473da58 into main Jun 19, 2026
2 checks passed
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.

Allow users to better control how many HttpClients are used

1 participant