[refactor] Revise HttpClient creation and caching.

[dizzzz]

Revise HttpClient creation and caching.

• reuse HttpClient when safe and possible
• this way we can reuse connection pooling over requests.
• add JMX monitoring

follow up for #6482

ToDo

• (p)review PR by DWES before un-Draft
• review by Joe/Claude
• introduced new 'http-version' attribute. Check if this is allowed in spec.

[joewiz]

(Clearing the original review since a new one is about ready.)

[dizzzz]

Thnx! I wonder why I added username/wordt to the record as this should be request based. It was probably to late :-) see more valid points, I will address these.

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Re-reviewing... The approach to caching and reusing the client (plus the JMX visibility) is a real improvement over building and tearing down a client per request. A few things stood out on a close read, grouped by severity. Line numbers are approximate.

Blocking — correctness

1. Evicted/expired clients are never closed (thread + connection leak). HttpClientFactory builds the cache with maximumSize(25) + expireAfterAccess(1h) but no removal listener, and the only place a client is closed is HttpClientCacheMonitor.reset() (JMX / shutdown hook). Caffeine eviction (size cap or hourly idle) drops the entry without calling HttpClient.close(), so each evicted java.net.http.HttpClient keeps its selector thread and keep-alive sockets until GC finalizes it. The previous code closed every client via try-with-resources, so this is a regression that undercuts the connection-reuse goal under any churning set of options. Suggest a Caffeine removalListener((key, client, cause) -> client.close()) on the builder, so every removal cause (size, expiry, explicit) closes the client — and then reset() can just invalidateAll().

2. Default HTTP version changed from negotiated HTTP/2 to pinned HTTP/1.1. newClient() now sets .version(options.httpVersion()), and HttpClientOptions.DEFAULTS is HTTP_1_1. The old buildClient() never called .version(), so it used the library default (HTTP/2, negotiating down to 1.1 via ALPN). No existing query sets the new http-version attribute, so every caller silently drops to HTTP/1.1 — a behavior change for anyone talking to an HTTP/2 endpoint. If 1.1 is intended as the new default, it's worth calling out explicitly; otherwise default to HTTP_2.

Should fix — contract / behavior

3. getHitRate() doesn't match its own contract or test. The HttpClientCacheMXBean javadoc says "Returns 0.0 when no requests have been made yet," but the implementation returns cache.stats().hitRate(), which Caffeine defines as 1.0 when requestCount() == 0. The test is named hitRateIsZeroWhenNoRequests yet asserts 1.0. So a cold cache reports a perfect hit rate, and the doc, the impl, and the test name disagree three ways. Either special-case requestCount() == 0 to return 0.0 (matching the doc) or fix the doc + test name to reflect the 1.0 Caffeine semantics.

4. auto-accept-encoding / http-version are read only from the eXist namespace. In RequestOptionsParser, follow-redirect is read with no namespace, but auto-accept-encoding and http-version are read only from Namespaces.EXIST_NS. A user who writes a bare (no-prefix) auto-accept-encoding="false" has it silently ignored (the autoAcceptEncodingNoNamespaceIgnored test pins this). If the namespace requirement is intentional for these eXist extensions, the asymmetry with follow-redirect is at least a documentation/usability footgun worth a note; if not, they should fall back to the no-namespace attribute like their sibling.

Lifecycle & integration

5. reset() can close clients out from under in-flight requests. reset() does cache.asMap().values().forEach(HttpClient::close) then invalidateAll(). Because clients are now shared, a request that obtained a client and is mid-send() will have it closed by a concurrent JMX reset() (or the shutdown hook), surfacing as HC001. Hard to fully avoid with shared clients, but worth being aware of — at minimum it argues for reset() being an explicit, rarely-used operation.

6. The JMX bean and the cache lifecycle sit outside eXist's management/lifecycle layer. HttpClientCacheMonitor.register() registers directly on ManagementFactory.getPlatformMBeanServer() with a fixed, non-instance-scoped ObjectName, and HttpClientFactory wires cleanup through a JVM Runtime shutdown hook over a JVM-global static cache. eXist already centralizes MBean registration and teardown in org.exist.management.Agent / AgentFactory (per-instance naming, unregistered in closeDBInstance). As written, in a multi-instance or embedded/test JVM (BrokerPool started and stopped repeatedly) the MBean and cache outlive the database instance, the second register() is a silent no-op pointing at a now-dead cache, and clients are only closed at JVM exit rather than on database shutdown. Registering through the Agent and tying reset() to the module/BrokerPool shutdown path would make this behave like the rest of eXist's JMX and free resources on restart.

7. getCacheSize() forces cache maintenance on every read. It calls cache.cleanUp() before estimatedSize(). cleanUp() drains Caffeine's eviction/maintenance work synchronously on the calling (JMX) thread, so a frequently-polling monitor repeatedly does write-path maintenance contending with request threads. estimatedSize() on its own is the intended cheap read.

Minor

8. Shared-client cache key — worth a guard/comment. Excluding credentials from the key is correct today because auth is applied per-HttpRequest, not on the client. But that invariant is unguarded: if a later change moves auth/proxy/SSL onto the client builder without adding it to HttpClientOptions, the cache would hand a client carrying one caller's auth to another. (Also: for connection-bound schemes like NTLM/Negotiate, a shared keep-alive connection can already carry auth state across callers.) A short comment on newClient() stating "configure nothing here that isn't part of the cache key" would protect it.

9. getCachedClientsSummary() omits httpVersion. It hand-concatenates followRedirect/timeout/autoAcceptEncoding, so two cached clients differing only in HTTP version look identical in JMX, and the string must be kept in sync as the record grows. Since HttpClientOptions is a record, opts.toString() already gives a faithful, self-maintaining rendering.

10. Small cleanups. UserCredentials has leftover scratch comments (//user, //) on its record components; RequestBuilder constructs an all-defaults RequestOptions inline where a RequestOptions.DEFAULTS constant (matching the per-component DEFAULTS) would read better; the three parseBooleanAttr overloads are a copy-paste chain that could collapse to one resolver.

The leak (1) and the version default (2) are the two I'd suggest be resolved before merge.

[dizzzz]

Claude:

6. The JMX bean and the cache lifecycle sit outside eXist's management/lifecycle layer. HttpClientCacheMonitor.register() registers directly on ManagementFactory.getPlatformMBeanServer() with a fixed, non-instance-scoped ObjectName, and HttpClientFactory wires cleanup through a JVM Runtime shutdown hook over a JVM-global static cache. eXist already centralizes MBean registration and teardown in org.exist.management.Agent / AgentFactory (per-instance naming, unregistered in closeDBInstance). As written, in a multi-instance or embedded/test JVM (BrokerPool started and stopped repeatedly) the MBean and cache outlive the database instance, the second register() is a silent no-op pointing at a now-dead cache, and clients are only closed at JVM exit rather than on database shutdown. Registering through the Agent and tying reset() to the module/BrokerPool shutdown path would make this behave like the rest of eXist's JMX and free resources on restart.