From 79cbc912513ba3a50f16b6bbcbd80dcfeba327c4 Mon Sep 17 00:00:00 2001 From: richard483 Date: Sat, 2 May 2026 22:13:20 +0700 Subject: [PATCH 1/4] feat(http): introduce RavenHttpClientFactory with shared connection pools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each @RavenApiClient previously instantiated its own reactor-netty HttpClient (and hence its own ConnectionProvider + LoopResources). For apps with many declared clients targeting one or two backends this multiplied event loops, selector threads, and connection pools without benefit. Introduce a public RavenHttpClientFactory extension point with a default implementation that shares an HttpClient (with its underlying ConnectionProvider and LoopResources) across all clients sharing a pool key — by default the scheme+host:port of the configured URL. Per-client opt-out is available via: nephren.raven.apiclient.configs..isolate-pool: true which routes that client through a private pool keyed by "isolated:". Use this when a client has materially different timeout/TLS/proxy needs from siblings, or its load profile would starve them. Shared-pool tunables are exposed under: nephren.raven.apiclient.shared-pool.max-connections (default 500) nephren.raven.apiclient.shared-pool.pending-acquire-max-count (default -1) The factory is wired in RavenApiConfiguration with @ConditionalOnMissingBean so users can replace it with a metric-instrumented or TLS-customized implementation. DefaultRavenHttpClientFactory implements DisposableBean so cached ConnectionProviders are released cleanly on context shutdown. Per-call timeouts are layered on top of the cached base HttpClient via .option/.doOnConnected; reactor-netty's HttpClient is copy-on-configure so this does not allocate new pool resources per call. The interceptor's prepareWebClient now resolves the pool key + timeouts and delegates to the factory; the local getHttpClient helper is removed. Adds DefaultRavenHttpClientFactoryTest covering: scheme+host:port pool keys, default port resolution, isolate-pool key shape, base-client caching (same key → one base, different keys → distinct), isolated clients bypassing the owned-provider cache, and DisposableBean cleanup. Co-Authored-By: Claude Opus 4.7 --- .../aop/RavenApiClientMethodInterceptor.java | 28 ++-- .../configuration/RavenApiConfiguration.java | 13 +- .../http/DefaultRavenHttpClientFactory.java | 95 +++++++++++++ .../raven/apiclient/http/PoolKeys.java | 39 +++++ .../http/RavenHttpClientFactory.java | 53 +++++++ .../properties/PropertiesHelper.java | 4 + .../properties/RavenApiClientProperties.java | 9 ++ .../properties/RavenSharedPoolProperties.java | 37 +++++ .../DefaultRavenHttpClientFactoryTest.java | 133 ++++++++++++++++++ 9 files changed, 390 insertions(+), 21 deletions(-) create mode 100644 src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java create mode 100644 src/main/java/com/nephren/raven/apiclient/http/PoolKeys.java create mode 100644 src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java create mode 100644 src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java create mode 100644 src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java diff --git a/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java b/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java index 5d5b113..6c24059 100644 --- a/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java +++ b/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java @@ -7,10 +7,8 @@ import com.nephren.raven.apiclient.aop.fallback.RavenApiClientFallback; import com.nephren.raven.apiclient.body.ApiBodyResolver; import com.nephren.raven.apiclient.errorresolver.ApiErrorResolver; +import com.nephren.raven.apiclient.http.RavenHttpClientFactory; import com.nephren.raven.apiclient.reactor.helper.SchedulerHelper; -import io.netty.channel.ChannelOption; -import io.netty.handler.timeout.ReadTimeoutHandler; -import io.netty.handler.timeout.WriteTimeoutHandler; import java.lang.reflect.Method; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; @@ -21,7 +19,6 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.concurrent.TimeUnit; import lombok.Setter; import lombok.extern.slf4j.Slf4j; import org.aopalliance.intercept.MethodInterceptor; @@ -46,7 +43,6 @@ import reactor.core.publisher.Mono; import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; -import reactor.netty.http.client.HttpClient; @Slf4j public class RavenApiClientMethodInterceptor implements InitializingBean, MethodInterceptor, @@ -92,9 +88,15 @@ private void prepareBodyResolvers() { } private void prepareWebClient() { + RavenHttpClientFactory factory = applicationContext.getBean(RavenHttpClientFactory.class); + String poolKey = RavenHttpClientFactory.poolKeyFor(name, metadata.getProperties()); + RavenHttpClientFactory.ConfigTimeouts timeouts = new RavenHttpClientFactory.ConfigTimeouts( + metadata.getProperties().getConnectTimeout(), + metadata.getProperties().getReadTimeout(), + metadata.getProperties().getWriteTimeout()); WebClient.Builder builder = applicationContext.getBean(WebClient.Builder.class) .exchangeStrategies(getExchangeStrategies()).baseUrl(metadata.getProperties().getUrl()) - .clientConnector(new ReactorClientHttpConnector(getHttpClient())) + .clientConnector(new ReactorClientHttpConnector(factory.httpClient(poolKey, timeouts))) .defaultHeaders( httpHeaders -> metadata.getProperties().getHeaders().forEach(httpHeaders::add)); webClient = builder.build(); @@ -149,20 +151,6 @@ private ExchangeStrategies getExchangeStrategies() { }).build(); } - private HttpClient getHttpClient() { - return HttpClient.create().option( - ChannelOption.CONNECT_TIMEOUT_MILLIS, - (int) metadata.getProperties().getConnectTimeout().toMillis()) - .doOnConnected(connection -> connection - .addHandlerLast( - new ReadTimeoutHandler(metadata.getProperties().getReadTimeout().toMillis(), - TimeUnit.MILLISECONDS)) - .addHandlerLast( - new WriteTimeoutHandler(metadata.getProperties().getWriteTimeout().toMillis(), - TimeUnit.MILLISECONDS)) - ); - } - @Override public Object invoke(MethodInvocation invocation) { Method method = invocation.getMethod(); diff --git a/src/main/java/com/nephren/raven/apiclient/configuration/RavenApiConfiguration.java b/src/main/java/com/nephren/raven/apiclient/configuration/RavenApiConfiguration.java index 83d9d44..eb7037a 100644 --- a/src/main/java/com/nephren/raven/apiclient/configuration/RavenApiConfiguration.java +++ b/src/main/java/com/nephren/raven/apiclient/configuration/RavenApiConfiguration.java @@ -4,7 +4,10 @@ import com.nephren.raven.apiclient.body.JsonBodyResolver; import com.nephren.raven.apiclient.body.MultipartBodyResolver; import com.nephren.raven.apiclient.errorresolver.DefaultApiErrorResolver; +import com.nephren.raven.apiclient.http.DefaultRavenHttpClientFactory; +import com.nephren.raven.apiclient.http.RavenHttpClientFactory; import com.nephren.raven.apiclient.properties.RavenApiClientProperties; +import com.nephren.raven.apiclient.properties.RavenSharedPoolProperties; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.context.annotation.Bean; @@ -14,7 +17,8 @@ @Configuration @Import(RavenApiClientRegistrar.class) @EnableConfigurationProperties({ - RavenApiClientProperties.class + RavenApiClientProperties.class, + RavenSharedPoolProperties.class }) public class RavenApiConfiguration { @@ -42,4 +46,11 @@ public DefaultApiErrorResolver defaultApiErrorResolver() { return new DefaultApiErrorResolver(); } + @Bean + @ConditionalOnMissingBean + public RavenHttpClientFactory ravenHttpClientFactory( + RavenSharedPoolProperties sharedPoolProperties) { + return new DefaultRavenHttpClientFactory(sharedPoolProperties); + } + } diff --git a/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java b/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java new file mode 100644 index 0000000..a375622 --- /dev/null +++ b/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java @@ -0,0 +1,95 @@ +package com.nephren.raven.apiclient.http; + +import com.nephren.raven.apiclient.properties.RavenSharedPoolProperties; +import io.netty.channel.ChannelOption; +import io.netty.handler.timeout.ReadTimeoutHandler; +import io.netty.handler.timeout.WriteTimeoutHandler; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; +import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.DisposableBean; +import reactor.netty.http.client.HttpClient; +import reactor.netty.resources.ConnectionProvider; + +/** + * Default {@link RavenHttpClientFactory} implementation. + * + *

Maintains a {@link ConcurrentHashMap} of base {@link HttpClient} instances keyed by + * pool key. Each base client owns a {@link ConnectionProvider} sized by + * {@link RavenSharedPoolProperties}; pool keys starting with {@code "isolated:"} bypass the + * shared provider and use Reactor Netty's default pool, giving the caller a private set of + * resources.

+ * + *

Per-call timeouts are layered on top of the cached base via {@code .option(...)} and + * {@code .doOnConnected(...)}; Reactor Netty's {@code HttpClient} is copy-on-configure, so + * those calls return a new wrapper that reuses the underlying pool/loop resources rather + * than allocating a fresh one.

+ * + *

Implements {@link DisposableBean} so the cached {@code ConnectionProvider}s release + * their resources cleanly on application context shutdown — important for tests and short- + * lived programs.

+ */ +@Slf4j +public class DefaultRavenHttpClientFactory implements RavenHttpClientFactory, DisposableBean { + + private final RavenSharedPoolProperties sharedPoolProperties; + private final Map baseClients = new ConcurrentHashMap<>(); + private final Map ownedProviders = new ConcurrentHashMap<>(); + + public DefaultRavenHttpClientFactory(RavenSharedPoolProperties sharedPoolProperties) { + this.sharedPoolProperties = sharedPoolProperties; + } + + @Override + public HttpClient httpClient(String poolKey, ConfigTimeouts timeouts) { + HttpClient base = baseClients.computeIfAbsent(poolKey, this::buildBase); + return base + .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, (int) timeouts.connect().toMillis()) + .doOnConnected(connection -> connection + .addHandlerLast(new ReadTimeoutHandler( + timeouts.read().toMillis(), TimeUnit.MILLISECONDS)) + .addHandlerLast(new WriteTimeoutHandler( + timeouts.write().toMillis(), TimeUnit.MILLISECONDS))); + } + + private HttpClient buildBase(String poolKey) { + if (poolKey.startsWith("isolated:")) { + log.debug("#RavenHttpClientFactory creating isolated HttpClient for {}", poolKey); + return HttpClient.create(); + } + ConnectionProvider provider = ConnectionProvider.builder("raven-" + sanitize(poolKey)) + .maxConnections(sharedPoolProperties.getMaxConnections()) + .pendingAcquireMaxCount(sharedPoolProperties.getPendingAcquireMaxCount()) + .build(); + ownedProviders.put(poolKey, provider); + log.debug( + "#RavenHttpClientFactory creating shared HttpClient for {} (maxConnections={}," + + " pendingAcquireMaxCount={})", + poolKey, + sharedPoolProperties.getMaxConnections(), + sharedPoolProperties.getPendingAcquireMaxCount()); + return HttpClient.create(provider); + } + + /** + * Reactor Netty {@code ConnectionProvider} names cannot contain certain characters; strip + * the URL-flavored ones so the provider name remains valid and human-readable. + */ + private static String sanitize(String poolKey) { + return poolKey.replace("://", "-").replace(":", "-").replace("/", "-"); + } + + @Override + public void destroy() { + ownedProviders.values().forEach(provider -> { + try { + provider.disposeLater().block(); + } catch (RuntimeException e) { + log.warn("#RavenHttpClientFactory failed to dispose connection provider", e); + } + }); + ownedProviders.clear(); + baseClients.clear(); + } +} diff --git a/src/main/java/com/nephren/raven/apiclient/http/PoolKeys.java b/src/main/java/com/nephren/raven/apiclient/http/PoolKeys.java new file mode 100644 index 0000000..c40eae3 --- /dev/null +++ b/src/main/java/com/nephren/raven/apiclient/http/PoolKeys.java @@ -0,0 +1,39 @@ +package com.nephren.raven.apiclient.http; + +import java.net.URI; +import java.net.URISyntaxException; + +/** + * Internal helpers for deriving stable pool keys from configured URLs. + * + *

The key shape is intentionally narrow — scheme + host + (resolved) port — so that two + * clients hitting the same logical backend share a pool regardless of differences in path, + * query, or trailing slashes. Anything we cannot parse falls back to the raw input string, + * which preserves backwards-compatible isolation for malformed URLs without throwing.

+ */ +final class PoolKeys { + + private PoolKeys() {} + + static String fromUrl(String url) { + if (url == null || url.isBlank()) { + return "default"; + } + try { + URI uri = new URI(url); + String scheme = uri.getScheme() == null ? "http" : uri.getScheme().toLowerCase(); + String host = uri.getHost(); + if (host == null) { + // not an absolute URL (e.g. just "localhost"); use the raw value as the key + return "raw:" + url; + } + int port = uri.getPort(); + if (port == -1) { + port = "https".equals(scheme) ? 443 : 80; + } + return scheme + "://" + host + ":" + port; + } catch (URISyntaxException e) { + return "raw:" + url; + } + } +} diff --git a/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java b/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java new file mode 100644 index 0000000..44e4e02 --- /dev/null +++ b/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java @@ -0,0 +1,53 @@ +package com.nephren.raven.apiclient.http; + +import com.nephren.raven.apiclient.properties.RavenApiClientProperties; +import java.time.Duration; +import reactor.netty.http.client.HttpClient; + +/** + * Extension point for materializing the Reactor Netty {@link HttpClient} that backs each + * {@code @RavenApiClient}'s {@code WebClient}. + * + *

The default implementation shares connection pools and event loops across all clients + * targeting the same scheme+host:port and applies the per-client connect/read/write timeouts + * on top. Replace this bean to take full control of pooling — typical reasons include custom + * TLS configuration, a metric-instrumented {@code ConnectionProvider}, or sharing pools with + * other parts of an application.

+ * + *

Implementations must be thread-safe; {@link #httpClient(String, ConfigTimeouts)} is + * called once at bean initialization time per declared client, but consumers may also call + * the returned {@link HttpClient} concurrently from any thread.

+ */ +public interface RavenHttpClientFactory { + + /** + * Resolve an {@link HttpClient} for the given client config and pool key. The factory is + * free to share an underlying {@code ConnectionProvider}/{@code LoopResources} between + * calls with the same {@code poolKey}; per-call configuration like timeouts is applied on + * top of the cached base client and does not affect the cache. + * + * @param poolKey identifies the pool. The default factory uses the scheme+host:port of + * the configured URL, or {@code "isolated:" + clientName} when the client + * is configured with {@code isolate-pool: true}. + * @param timeouts per-call connect/read/write timeouts to apply to the returned client. + * @return an {@link HttpClient} configured with the given timeouts; the underlying + * connection pool may be shared across calls with the same {@code poolKey}. + */ + HttpClient httpClient(String poolKey, ConfigTimeouts timeouts); + + /** + * Helper that derives the pool key from a single client's config: the scheme+host:port of + * its configured URL by default, or {@code "isolated:" + clientName} when + * {@link RavenApiClientProperties.ApiClientConfigProperties#isIsolatePool()} is set. + */ + static String poolKeyFor(String clientName, + RavenApiClientProperties.ApiClientConfigProperties config) { + if (config.isIsolatePool()) { + return "isolated:" + clientName; + } + return PoolKeys.fromUrl(config.getUrl()); + } + + /** Timeouts applied to the returned {@link HttpClient}. */ + record ConfigTimeouts(Duration connect, Duration read, Duration write) {} +} diff --git a/src/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.java b/src/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.java index c3af39c..eb2a7a9 100644 --- a/src/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.java +++ b/src/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.java @@ -33,6 +33,10 @@ public static void copyConfigPropertiesFromSourceToTarget( target.setErrorResolver(source.getErrorResolver()); } + // boolean primitive — always copy. Default field value (false) makes this safe even + // when the source did not set it explicitly. + target.setIsolatePool(source.isIsolatePool()); + source.getHeaders().forEach((key, value) -> target.getHeaders().put(key, value)); } } diff --git a/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java b/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java index 8262bc1..74d0287 100644 --- a/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java +++ b/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java @@ -49,6 +49,15 @@ public static class ApiClientConfigProperties { private Map headers = new HashMap<>(); private Class errorResolver; + + /** + * When {@code true}, this client gets its own Reactor Netty {@code HttpClient} (and hence + * its own connection pool and event loops) instead of sharing the pool keyed by + * scheme+host:port. Default is {@code false} — most callers benefit from sharing. Set to + * {@code true} when this client has materially different timeout/TLS/proxy needs from + * other clients targeting the same host, or when its load profile would starve them. + */ + private boolean isolatePool = false; } } \ No newline at end of file diff --git a/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java b/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java new file mode 100644 index 0000000..a357d00 --- /dev/null +++ b/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java @@ -0,0 +1,37 @@ +package com.nephren.raven.apiclient.properties; + +import lombok.AllArgsConstructor; +import lombok.Data; +import lombok.NoArgsConstructor; +import org.springframework.boot.context.properties.ConfigurationProperties; + +/** + * Tunables for the Reactor Netty {@code ConnectionProvider} backing every shared + * {@code HttpClient} pool. Most apps will never need to touch these — they are exposed + * primarily for environments with high client fan-out or unusually slow downstreams. + * + *

These values apply to every shared pool the factory creates (one per scheme+host:port + * key). Per-client opt-out via {@code isolate-pool} is configured on each entry in + * {@code nephren.raven.apiclient.configs.}. + */ +@Data +@AllArgsConstructor +@NoArgsConstructor +@ConfigurationProperties("nephren.raven.apiclient.shared-pool") +public class RavenSharedPoolProperties { + + /** + * Maximum number of concurrent connections kept open per pool. Defaults to 500, well above + * Reactor Netty's library default (which is small) so the shared-pool experience matches + * what callers used to get when each client owned its own pool. + */ + private int maxConnections = 500; + + /** + * Maximum number of acquire attempts allowed to queue while the pool is saturated. {@code -1} + * means unbounded (Reactor Netty default). Set a positive value to fail-fast under load + * instead of letting backpressure pile up. + */ + private int pendingAcquireMaxCount = -1; + +} diff --git a/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java b/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java new file mode 100644 index 0000000..fdfeea6 --- /dev/null +++ b/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java @@ -0,0 +1,133 @@ +package com.nephren.raven.apiclient.unit; + +import com.nephren.raven.apiclient.http.DefaultRavenHttpClientFactory; +import com.nephren.raven.apiclient.http.RavenHttpClientFactory; +import com.nephren.raven.apiclient.properties.RavenApiClientProperties; +import com.nephren.raven.apiclient.properties.RavenSharedPoolProperties; +import java.time.Duration; +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import reactor.netty.http.client.HttpClient; + +class DefaultRavenHttpClientFactoryTest { + + private DefaultRavenHttpClientFactory factory; + + @BeforeEach + void setUp() { + factory = new DefaultRavenHttpClientFactory(new RavenSharedPoolProperties()); + } + + @AfterEach + void tearDown() { + factory.destroy(); + } + + private static RavenHttpClientFactory.ConfigTimeouts timeouts() { + return new RavenHttpClientFactory.ConfigTimeouts( + Duration.ofMillis(2000), Duration.ofMillis(2000), Duration.ofMillis(2000)); + } + + @Test + void poolKeyFor_byDefault_usesSchemeHostPort() { + RavenApiClientProperties.ApiClientConfigProperties config = + new RavenApiClientProperties.ApiClientConfigProperties(); + config.setUrl("http://example.com:8080/api"); + + String key = RavenHttpClientFactory.poolKeyFor("any-name", config); + + Assertions.assertThat(key).isEqualTo("http://example.com:8080"); + } + + @Test + void poolKeyFor_resolvesDefaultPortPerScheme() { + RavenApiClientProperties.ApiClientConfigProperties http = + new RavenApiClientProperties.ApiClientConfigProperties(); + http.setUrl("http://example.com"); + RavenApiClientProperties.ApiClientConfigProperties https = + new RavenApiClientProperties.ApiClientConfigProperties(); + https.setUrl("https://example.com"); + + Assertions.assertThat(RavenHttpClientFactory.poolKeyFor("a", http)) + .isEqualTo("http://example.com:80"); + Assertions.assertThat(RavenHttpClientFactory.poolKeyFor("b", https)) + .isEqualTo("https://example.com:443"); + } + + @Test + void poolKeyFor_whenIsolatePoolSet_returnsIsolatedKeyWithClientName() { + RavenApiClientProperties.ApiClientConfigProperties config = + new RavenApiClientProperties.ApiClientConfigProperties(); + config.setUrl("http://shared-host:8080"); + config.setIsolatePool(true); + + String key = RavenHttpClientFactory.poolKeyFor("specialClient", config); + + Assertions.assertThat(key).isEqualTo("isolated:specialClient"); + } + + @Test + void httpClient_withSamePoolKey_returnsConfigurationsThatShareUnderlyingResources() { + HttpClient first = factory.httpClient("http://example.com:8080", timeouts()); + HttpClient second = factory.httpClient("http://example.com:8080", timeouts()); + + // Reactor Netty HttpClient is copy-on-configure, so the per-call layer (.option/.doOnConnected) + // makes the returned wrappers non-identical; the cached base client is what the factory pools. + Assertions.assertThat(first).isNotNull(); + Assertions.assertThat(second).isNotNull(); + // The factory returns wrappers per call, so we cannot compare references directly. We assert + // instead that the cache size stays at 1 entry — i.e. the second call did not allocate a + // second base client / ConnectionProvider. + factory.httpClient("http://example.com:8080", timeouts()); + factory.httpClient("http://example.com:8080", timeouts()); + Assertions.assertThat(factory) + .extracting("baseClients", org.assertj.core.api.InstanceOfAssertFactories.MAP) + .hasSize(1); + } + + @Test + void httpClient_withDifferentPoolKeys_allocatesDistinctBaseClients() { + factory.httpClient("http://host-a:80", timeouts()); + factory.httpClient("http://host-b:80", timeouts()); + + Assertions.assertThat(factory) + .extracting("baseClients", org.assertj.core.api.InstanceOfAssertFactories.MAP) + .hasSize(2); + } + + @Test + void isolatedPoolKey_doesNotCreateAnOwnedConnectionProvider() { + factory.httpClient("isolated:special", timeouts()); + + Assertions.assertThat(factory) + .extracting("baseClients", org.assertj.core.api.InstanceOfAssertFactories.MAP) + .hasSize(1); + Assertions.assertThat(factory) + .extracting("ownedProviders", org.assertj.core.api.InstanceOfAssertFactories.MAP) + .isEmpty(); + } + + @Test + void sharedPoolKey_createsOwnedConnectionProvider() { + factory.httpClient("http://example.com:80", timeouts()); + + Assertions.assertThat(factory) + .extracting("ownedProviders", org.assertj.core.api.InstanceOfAssertFactories.MAP) + .hasSize(1); + } + + @Test + void destroy_clearsAllCachedState() { + factory.httpClient("http://host-a:80", timeouts()); + factory.httpClient("isolated:special", timeouts()); + + factory.destroy(); + + Assertions.assertThat(factory) + .extracting("baseClients", org.assertj.core.api.InstanceOfAssertFactories.MAP).isEmpty(); + Assertions.assertThat(factory) + .extracting("ownedProviders", org.assertj.core.api.InstanceOfAssertFactories.MAP).isEmpty(); + } +} From 1214d91fcad53462083f22631459b2c5d1dad6c7 Mon Sep 17 00:00:00 2001 From: richard483 Date: Sat, 2 May 2026 22:33:43 +0700 Subject: [PATCH 2/4] =?UTF-8?q?fix(http):=20address=20PR=20#21=20review=20?= =?UTF-8?q?=E2=80=94=20true=20isolation,=20channel-handler=20safety,=20inh?= =?UTF-8?q?eritance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses seven review comments on PR #21: #5/#7 (CodeRabbit, Copilot) — Isolated keys previously called HttpClient.create() with no provider, which uses Reactor Netty's process-global ConnectionProvider. Two clients with isolate-pool: true would still contend on the same global 500-connection pool, contradicting the documented "private set of resources" guarantee. Fix: every key — isolated or shared — now owns a dedicated ConnectionProvider tracked in ownedProviders and disposed on shutdown. #2 (Copilot) — ReadTimeoutHandler / WriteTimeoutHandler attach to the Netty channel; a connection created under client A and later reused by client B would carry A's timeout handlers. The default pool key now includes the connect/read/write timeout fingerprint (scheme://host:port|c=...,r=...,w=...), so two clients only share a pooled channel when they have identical timeouts. Documented the trade-off in DefaultRavenHttpClientFactory's javadoc — custom factories that apply timeouts at the request level (e.g. responseTimeout) may use a narrower key. #4 (Copilot) — isolatePool was a primitive boolean with default false, which the merge helper unconditionally copied. A value set on configs.default.isolate-pool would therefore be silently overwritten back to false by any named config that omitted the key. Switched to Boolean (nullable) and updated PropertiesHelper to copy only when the source has it set, matching the inheritance pattern used by every other optional field. #1 (Copilot) — The factory contract narrowed everything down to (poolKey, ConfigTimeouts) before the impl saw it, which prevented custom factories from making per-client decisions based on URL, headers, or any other property. Widened to httpClient(String clientName, ApiClientConfigProperties config); the default implementation derives its pool key via the static helper RavenHttpClientFactory.defaultPoolKey but custom factories can use any keying strategy they want. Done now (before merge) so the public contract is right from day one. #6 (CodeRabbit) — disposeLater().block() was unbounded; a stalled ConnectionProvider could hang Spring context shutdown indefinitely. Bounded to a 30-second timeout per provider; failures are logged with the pool key and the loop continues for the remaining providers. #3 (Copilot) — docs/setup/index.md previously enumerated every supported nephren.raven.apiclient.* key; updated to document configs..isolate-pool and the new shared-pool.* tunables. Tests updated: - defaultPoolKey assertions now expect the timeouts fingerprint suffix. - New test confirming same host + different timeouts → distinct base clients (the regression #2 would have masked). - New test confirming two isolated clients on the same backend each own their own ConnectionProvider entry — the regression #5/#7 would have made this fail silently. Co-Authored-By: Claude Opus 4.7 --- docs/setup/index.md | 20 +++ .../aop/RavenApiClientMethodInterceptor.java | 9 +- .../http/DefaultRavenHttpClientFactory.java | 59 ++++---- .../http/RavenHttpClientFactory.java | 67 +++++---- .../properties/PropertiesHelper.java | 6 +- .../properties/RavenApiClientProperties.java | 14 +- .../DefaultRavenHttpClientFactoryTest.java | 133 +++++++++--------- 7 files changed, 176 insertions(+), 132 deletions(-) diff --git a/docs/setup/index.md b/docs/setup/index.md index 7af0117..9e64939 100644 --- a/docs/setup/index.md +++ b/docs/setup/index.md @@ -35,6 +35,26 @@ nephren.raven.apiclient.configs..headers.=applicati # not required, error resolver for the api client, default is DefaultErrorResolver nephren.raven.apiclient.configs..error-resolver=com.nephren.raven.apiclient. serviceExample.client.errorresolver.DefaultErrorResolver + +# not required, opt this client out of shared connection pooling so it gets its own +# dedicated Reactor Netty HttpClient + ConnectionProvider, default is false. By default +# clients sharing a scheme://host:port (and identical timeouts) reuse the same pool. +nephren.raven.apiclient.configs..isolate-pool=true +``` + +## Shared Connection Pool Tunables + +These apply to every shared `HttpClient` pool the default factory creates. Most apps will +never need to touch them — defaults match Reactor Netty conventions and are sized for +typical client fan-out. + +``` +# not required, max concurrent connections per pool, default is 500 +nephren.raven.apiclient.shared-pool.max-connections=500 + +# not required, max queued acquire attempts when the pool is saturated. +# -1 (default) is unbounded; set a positive value to fail-fast under load. +nephren.raven.apiclient.shared-pool.pending-acquire-max-count=-1 ``` ## Api Scheduler diff --git a/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java b/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java index 6c24059..9dff82d 100644 --- a/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java +++ b/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java @@ -43,6 +43,7 @@ import reactor.core.publisher.Mono; import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; +import reactor.netty.http.client.HttpClient; @Slf4j public class RavenApiClientMethodInterceptor implements InitializingBean, MethodInterceptor, @@ -89,14 +90,10 @@ private void prepareBodyResolvers() { private void prepareWebClient() { RavenHttpClientFactory factory = applicationContext.getBean(RavenHttpClientFactory.class); - String poolKey = RavenHttpClientFactory.poolKeyFor(name, metadata.getProperties()); - RavenHttpClientFactory.ConfigTimeouts timeouts = new RavenHttpClientFactory.ConfigTimeouts( - metadata.getProperties().getConnectTimeout(), - metadata.getProperties().getReadTimeout(), - metadata.getProperties().getWriteTimeout()); + HttpClient httpClient = factory.httpClient(name, metadata.getProperties()); WebClient.Builder builder = applicationContext.getBean(WebClient.Builder.class) .exchangeStrategies(getExchangeStrategies()).baseUrl(metadata.getProperties().getUrl()) - .clientConnector(new ReactorClientHttpConnector(factory.httpClient(poolKey, timeouts))) + .clientConnector(new ReactorClientHttpConnector(httpClient)) .defaultHeaders( httpHeaders -> metadata.getProperties().getHeaders().forEach(httpHeaders::add)); webClient = builder.build(); diff --git a/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java b/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java index a375622..4fa7bf1 100644 --- a/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java +++ b/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java @@ -1,9 +1,11 @@ package com.nephren.raven.apiclient.http; +import com.nephren.raven.apiclient.properties.RavenApiClientProperties.ApiClientConfigProperties; import com.nephren.raven.apiclient.properties.RavenSharedPoolProperties; import io.netty.channel.ChannelOption; import io.netty.handler.timeout.ReadTimeoutHandler; import io.netty.handler.timeout.WriteTimeoutHandler; +import java.time.Duration; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; @@ -16,23 +18,28 @@ * Default {@link RavenHttpClientFactory} implementation. * *

Maintains a {@link ConcurrentHashMap} of base {@link HttpClient} instances keyed by - * pool key. Each base client owns a {@link ConnectionProvider} sized by - * {@link RavenSharedPoolProperties}; pool keys starting with {@code "isolated:"} bypass the - * shared provider and use Reactor Netty's default pool, giving the caller a private set of - * resources.

+ * pool key. Each base owns a dedicated {@link ConnectionProvider} sized by + * {@link RavenSharedPoolProperties}; isolated keys ({@code "isolated:"}) get + * their own provider too — they are isolated from each other as well as from the + * shared pools, which would not be the case if the implementation fell back to + * {@code HttpClient.create()} (that uses Reactor Netty's process-global pool).

* - *

Per-call timeouts are layered on top of the cached base via {@code .option(...)} and - * {@code .doOnConnected(...)}; Reactor Netty's {@code HttpClient} is copy-on-configure, so - * those calls return a new wrapper that reuses the underlying pool/loop resources rather - * than allocating a fresh one.

+ *

The pool key folds the configured timeouts in because the + * {@link ReadTimeoutHandler} / {@link WriteTimeoutHandler} we install attach at the channel + * level. Sharing a pooled channel between clients with different timeouts would mean the + * second client gets the first one's handlers, which is silently incorrect. Pool reuse + * therefore happens only between clients that target the same backend with the same + * timeouts — typically the entire application.

* - *

Implements {@link DisposableBean} so the cached {@code ConnectionProvider}s release - * their resources cleanly on application context shutdown — important for tests and short- - * lived programs.

+ *

Implements {@link DisposableBean} so the cached {@link ConnectionProvider}s release + * their resources on application context shutdown. Disposal is time-bounded so a stuck + * provider cannot hang the shutdown phase.

*/ @Slf4j public class DefaultRavenHttpClientFactory implements RavenHttpClientFactory, DisposableBean { + private static final Duration DISPOSE_TIMEOUT = Duration.ofSeconds(30); + private final RavenSharedPoolProperties sharedPoolProperties; private final Map baseClients = new ConcurrentHashMap<>(); private final Map ownedProviders = new ConcurrentHashMap<>(); @@ -42,29 +49,27 @@ public DefaultRavenHttpClientFactory(RavenSharedPoolProperties sharedPoolPropert } @Override - public HttpClient httpClient(String poolKey, ConfigTimeouts timeouts) { + public HttpClient httpClient(String clientName, ApiClientConfigProperties config) { + String poolKey = RavenHttpClientFactory.defaultPoolKey(clientName, config); HttpClient base = baseClients.computeIfAbsent(poolKey, this::buildBase); return base - .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, (int) timeouts.connect().toMillis()) + .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, + (int) config.getConnectTimeout().toMillis()) .doOnConnected(connection -> connection .addHandlerLast(new ReadTimeoutHandler( - timeouts.read().toMillis(), TimeUnit.MILLISECONDS)) + config.getReadTimeout().toMillis(), TimeUnit.MILLISECONDS)) .addHandlerLast(new WriteTimeoutHandler( - timeouts.write().toMillis(), TimeUnit.MILLISECONDS))); + config.getWriteTimeout().toMillis(), TimeUnit.MILLISECONDS))); } private HttpClient buildBase(String poolKey) { - if (poolKey.startsWith("isolated:")) { - log.debug("#RavenHttpClientFactory creating isolated HttpClient for {}", poolKey); - return HttpClient.create(); - } ConnectionProvider provider = ConnectionProvider.builder("raven-" + sanitize(poolKey)) .maxConnections(sharedPoolProperties.getMaxConnections()) .pendingAcquireMaxCount(sharedPoolProperties.getPendingAcquireMaxCount()) .build(); ownedProviders.put(poolKey, provider); log.debug( - "#RavenHttpClientFactory creating shared HttpClient for {} (maxConnections={}," + "#RavenHttpClientFactory creating HttpClient for {} (maxConnections={}," + " pendingAcquireMaxCount={})", poolKey, sharedPoolProperties.getMaxConnections(), @@ -77,16 +82,22 @@ private HttpClient buildBase(String poolKey) { * the URL-flavored ones so the provider name remains valid and human-readable. */ private static String sanitize(String poolKey) { - return poolKey.replace("://", "-").replace(":", "-").replace("/", "-"); + return poolKey + .replace("://", "-") + .replace(":", "-") + .replace("/", "-") + .replace("|", "-") + .replace(",", "-") + .replace("=", "-"); } @Override public void destroy() { - ownedProviders.values().forEach(provider -> { + ownedProviders.forEach((key, provider) -> { try { - provider.disposeLater().block(); + provider.disposeLater().block(DISPOSE_TIMEOUT); } catch (RuntimeException e) { - log.warn("#RavenHttpClientFactory failed to dispose connection provider", e); + log.warn("#RavenHttpClientFactory failed to dispose connection provider for {}", key, e); } }); ownedProviders.clear(); diff --git a/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java b/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java index 44e4e02..8ff2301 100644 --- a/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java +++ b/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java @@ -1,7 +1,6 @@ package com.nephren.raven.apiclient.http; -import com.nephren.raven.apiclient.properties.RavenApiClientProperties; -import java.time.Duration; +import com.nephren.raven.apiclient.properties.RavenApiClientProperties.ApiClientConfigProperties; import reactor.netty.http.client.HttpClient; /** @@ -9,45 +8,53 @@ * {@code @RavenApiClient}'s {@code WebClient}. * *

The default implementation shares connection pools and event loops across all clients - * targeting the same scheme+host:port and applies the per-client connect/read/write timeouts - * on top. Replace this bean to take full control of pooling — typical reasons include custom - * TLS configuration, a metric-instrumented {@code ConnectionProvider}, or sharing pools with - * other parts of an application.

+ * targeting the same scheme+host:port (with the same configured timeouts) and applies the + * per-client connect/read/write timeouts on top. Replace this bean to take full control of + * pooling — typical reasons include custom TLS configuration, a metric-instrumented + * {@code ConnectionProvider}, sharing pools with other parts of an application, or a + * different keying strategy.

* - *

Implementations must be thread-safe; {@link #httpClient(String, ConfigTimeouts)} is - * called once at bean initialization time per declared client, but consumers may also call - * the returned {@link HttpClient} concurrently from any thread.

+ *

The full per-client configuration is passed to {@link #httpClient(String, + * ApiClientConfigProperties)} so a replacement implementation can make decisions based on + * URL, headers, fallback class, or any other property. Implementations must be thread-safe; + * {@link #httpClient} is called once per declared client at bean initialization, but the + * returned {@link HttpClient} may be invoked concurrently from any thread.

*/ public interface RavenHttpClientFactory { /** - * Resolve an {@link HttpClient} for the given client config and pool key. The factory is - * free to share an underlying {@code ConnectionProvider}/{@code LoopResources} between - * calls with the same {@code poolKey}; per-call configuration like timeouts is applied on - * top of the cached base client and does not affect the cache. + * Resolve an {@link HttpClient} for the given client. Implementations are free to share + * an underlying {@code ConnectionProvider}/{@code LoopResources} between calls; the + * default implementation does so when {@code config} resolves to the same pool key. * - * @param poolKey identifies the pool. The default factory uses the scheme+host:port of - * the configured URL, or {@code "isolated:" + clientName} when the client - * is configured with {@code isolate-pool: true}. - * @param timeouts per-call connect/read/write timeouts to apply to the returned client. - * @return an {@link HttpClient} configured with the given timeouts; the underlying - * connection pool may be shared across calls with the same {@code poolKey}. + * @param clientName the {@code @RavenApiClient(name = ...)} value, used by the default + * implementation when {@code isolate-pool} is set. + * @param config the merged {@link ApiClientConfigProperties} for this client (after + * defaults are applied), giving the implementation access to URL, + * timeouts, headers, etc. + * @return an {@link HttpClient} ready for use; the underlying connection pool may be + * shared with other clients depending on the factory's keying strategy. */ - HttpClient httpClient(String poolKey, ConfigTimeouts timeouts); + HttpClient httpClient(String clientName, ApiClientConfigProperties config); /** - * Helper that derives the pool key from a single client's config: the scheme+host:port of - * its configured URL by default, or {@code "isolated:" + clientName} when - * {@link RavenApiClientProperties.ApiClientConfigProperties#isIsolatePool()} is set. + * The default pool key the bundled implementation uses: {@code scheme://host:port|} + * for shared pools, or {@code "isolated:" + clientName} when the client is configured with + * {@code isolate-pool: true}. + * + *

Timeouts are folded into the shared key because Reactor Netty's + * {@code ReadTimeoutHandler} / {@code WriteTimeoutHandler} attach to the channel, so two + * clients with different read/write timeouts cannot safely share a pooled channel. Custom + * factories that apply timeouts at the request level (e.g. via {@code responseTimeout}) + * may use a narrower key.

*/ - static String poolKeyFor(String clientName, - RavenApiClientProperties.ApiClientConfigProperties config) { - if (config.isIsolatePool()) { + static String defaultPoolKey(String clientName, ApiClientConfigProperties config) { + if (Boolean.TRUE.equals(config.getIsolatePool())) { return "isolated:" + clientName; } - return PoolKeys.fromUrl(config.getUrl()); + return PoolKeys.fromUrl(config.getUrl()) + + "|c=" + config.getConnectTimeout().toMillis() + + ",r=" + config.getReadTimeout().toMillis() + + ",w=" + config.getWriteTimeout().toMillis(); } - - /** Timeouts applied to the returned {@link HttpClient}. */ - record ConfigTimeouts(Duration connect, Duration read, Duration write) {} } diff --git a/src/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.java b/src/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.java index eb2a7a9..c40c692 100644 --- a/src/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.java +++ b/src/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.java @@ -33,9 +33,9 @@ public static void copyConfigPropertiesFromSourceToTarget( target.setErrorResolver(source.getErrorResolver()); } - // boolean primitive — always copy. Default field value (false) makes this safe even - // when the source did not set it explicitly. - target.setIsolatePool(source.isIsolatePool()); + if (Objects.nonNull(source.getIsolatePool())) { + target.setIsolatePool(source.getIsolatePool()); + } source.getHeaders().forEach((key, value) -> target.getHeaders().put(key, value)); } diff --git a/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java b/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java index 74d0287..a6311ce 100644 --- a/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java +++ b/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java @@ -53,11 +53,17 @@ public static class ApiClientConfigProperties { /** * When {@code true}, this client gets its own Reactor Netty {@code HttpClient} (and hence * its own connection pool and event loops) instead of sharing the pool keyed by - * scheme+host:port. Default is {@code false} — most callers benefit from sharing. Set to - * {@code true} when this client has materially different timeout/TLS/proxy needs from - * other clients targeting the same host, or when its load profile would starve them. + * scheme+host:port. Treated as {@code false} when unset (most callers benefit from + * sharing). Set to {@code true} when this client has materially different TLS/proxy + * needs from other clients targeting the same host, or when its load profile would + * starve them. + * + *

Modeled as the wrapper {@link Boolean} so that a value set on + * {@code configs.default.isolate-pool} can be inherited by named configs that omit the + * key — a primitive default would unconditionally overwrite the inherited value back to + * {@code false} during property merging.

*/ - private boolean isolatePool = false; + private Boolean isolatePool; } } \ No newline at end of file diff --git a/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java b/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java index fdfeea6..82a758f 100644 --- a/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java +++ b/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java @@ -6,10 +6,10 @@ import com.nephren.raven.apiclient.properties.RavenSharedPoolProperties; import java.time.Duration; import org.assertj.core.api.Assertions; +import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import reactor.netty.http.client.HttpClient; class DefaultRavenHttpClientFactoryTest { @@ -25,109 +25,112 @@ void tearDown() { factory.destroy(); } - private static RavenHttpClientFactory.ConfigTimeouts timeouts() { - return new RavenHttpClientFactory.ConfigTimeouts( - Duration.ofMillis(2000), Duration.ofMillis(2000), Duration.ofMillis(2000)); - } - - @Test - void poolKeyFor_byDefault_usesSchemeHostPort() { - RavenApiClientProperties.ApiClientConfigProperties config = + private static RavenApiClientProperties.ApiClientConfigProperties config(String url) { + RavenApiClientProperties.ApiClientConfigProperties c = new RavenApiClientProperties.ApiClientConfigProperties(); - config.setUrl("http://example.com:8080/api"); - - String key = RavenHttpClientFactory.poolKeyFor("any-name", config); + c.setUrl(url); + return c; + } - Assertions.assertThat(key).isEqualTo("http://example.com:8080"); + private static RavenApiClientProperties.ApiClientConfigProperties config( + String url, long connectMs, long readMs, long writeMs) { + RavenApiClientProperties.ApiClientConfigProperties c = config(url); + c.setConnectTimeout(Duration.ofMillis(connectMs)); + c.setReadTimeout(Duration.ofMillis(readMs)); + c.setWriteTimeout(Duration.ofMillis(writeMs)); + return c; } @Test - void poolKeyFor_resolvesDefaultPortPerScheme() { - RavenApiClientProperties.ApiClientConfigProperties http = - new RavenApiClientProperties.ApiClientConfigProperties(); - http.setUrl("http://example.com"); - RavenApiClientProperties.ApiClientConfigProperties https = - new RavenApiClientProperties.ApiClientConfigProperties(); - https.setUrl("https://example.com"); - - Assertions.assertThat(RavenHttpClientFactory.poolKeyFor("a", http)) - .isEqualTo("http://example.com:80"); - Assertions.assertThat(RavenHttpClientFactory.poolKeyFor("b", https)) - .isEqualTo("https://example.com:443"); + void defaultPoolKey_byDefault_usesSchemeHostPortPlusTimeoutFingerprint() { + String key = RavenHttpClientFactory.defaultPoolKey( + "any-name", config("http://example.com:8080/api", 1000, 2000, 3000)); + Assertions.assertThat(key).isEqualTo("http://example.com:8080|c=1000,r=2000,w=3000"); } @Test - void poolKeyFor_whenIsolatePoolSet_returnsIsolatedKeyWithClientName() { - RavenApiClientProperties.ApiClientConfigProperties config = - new RavenApiClientProperties.ApiClientConfigProperties(); - config.setUrl("http://shared-host:8080"); - config.setIsolatePool(true); + void defaultPoolKey_resolvesDefaultPortPerScheme() { + Assertions.assertThat( + RavenHttpClientFactory.defaultPoolKey("a", config("http://example.com"))) + .startsWith("http://example.com:80|"); + Assertions.assertThat( + RavenHttpClientFactory.defaultPoolKey("b", config("https://example.com"))) + .startsWith("https://example.com:443|"); + } - String key = RavenHttpClientFactory.poolKeyFor("specialClient", config); + @Test + void defaultPoolKey_whenIsolatePoolSet_returnsIsolatedKeyWithClientName() { + RavenApiClientProperties.ApiClientConfigProperties c = config("http://shared-host:8080"); + c.setIsolatePool(true); - Assertions.assertThat(key).isEqualTo("isolated:specialClient"); + Assertions.assertThat(RavenHttpClientFactory.defaultPoolKey("specialClient", c)) + .isEqualTo("isolated:specialClient"); } @Test - void httpClient_withSamePoolKey_returnsConfigurationsThatShareUnderlyingResources() { - HttpClient first = factory.httpClient("http://example.com:8080", timeouts()); - HttpClient second = factory.httpClient("http://example.com:8080", timeouts()); - - // Reactor Netty HttpClient is copy-on-configure, so the per-call layer (.option/.doOnConnected) - // makes the returned wrappers non-identical; the cached base client is what the factory pools. - Assertions.assertThat(first).isNotNull(); - Assertions.assertThat(second).isNotNull(); - // The factory returns wrappers per call, so we cannot compare references directly. We assert - // instead that the cache size stays at 1 entry — i.e. the second call did not allocate a - // second base client / ConnectionProvider. - factory.httpClient("http://example.com:8080", timeouts()); - factory.httpClient("http://example.com:8080", timeouts()); + void httpClient_withSamePoolKey_sharesUnderlyingBaseClient() { + factory.httpClient("a", config("http://example.com:8080", 2000, 2000, 2000)); + factory.httpClient("b", config("http://example.com:8080", 2000, 2000, 2000)); + factory.httpClient("c", config("http://example.com:8080", 2000, 2000, 2000)); + Assertions.assertThat(factory) - .extracting("baseClients", org.assertj.core.api.InstanceOfAssertFactories.MAP) + .extracting("baseClients", InstanceOfAssertFactories.MAP) .hasSize(1); } @Test - void httpClient_withDifferentPoolKeys_allocatesDistinctBaseClients() { - factory.httpClient("http://host-a:80", timeouts()); - factory.httpClient("http://host-b:80", timeouts()); + void httpClient_withDifferentHosts_allocatesDistinctBaseClients() { + factory.httpClient("a", config("http://host-a:80", 2000, 2000, 2000)); + factory.httpClient("b", config("http://host-b:80", 2000, 2000, 2000)); Assertions.assertThat(factory) - .extracting("baseClients", org.assertj.core.api.InstanceOfAssertFactories.MAP) + .extracting("baseClients", InstanceOfAssertFactories.MAP) .hasSize(2); } @Test - void isolatedPoolKey_doesNotCreateAnOwnedConnectionProvider() { - factory.httpClient("isolated:special", timeouts()); + void httpClient_withSameHostButDifferentTimeouts_allocatesDistinctBaseClients() { + // Channel-bound ReadTimeoutHandler/WriteTimeoutHandler would otherwise leak across + // clients sharing a pooled channel; the default key folds timeouts in to prevent it. + factory.httpClient("a", config("http://example.com:80", 2000, 2000, 2000)); + factory.httpClient("b", config("http://example.com:80", 5000, 5000, 5000)); Assertions.assertThat(factory) - .extracting("baseClients", org.assertj.core.api.InstanceOfAssertFactories.MAP) - .hasSize(1); - Assertions.assertThat(factory) - .extracting("ownedProviders", org.assertj.core.api.InstanceOfAssertFactories.MAP) - .isEmpty(); + .extracting("baseClients", InstanceOfAssertFactories.MAP) + .hasSize(2); } @Test - void sharedPoolKey_createsOwnedConnectionProvider() { - factory.httpClient("http://example.com:80", timeouts()); + void isolatedClients_eachOwnTheirOwnConnectionProvider() { + RavenApiClientProperties.ApiClientConfigProperties a = config("http://shared:80"); + a.setIsolatePool(true); + RavenApiClientProperties.ApiClientConfigProperties b = config("http://shared:80"); + b.setIsolatePool(true); + factory.httpClient("client-a", a); + factory.httpClient("client-b", b); + + // Two isolated clients on the same backend must NOT contend on a single pool — + // verify each got its own owned ConnectionProvider entry. Assertions.assertThat(factory) - .extracting("ownedProviders", org.assertj.core.api.InstanceOfAssertFactories.MAP) - .hasSize(1); + .extracting("ownedProviders", InstanceOfAssertFactories.MAP) + .hasSize(2) + .extracting(java.util.Map::keySet, InstanceOfAssertFactories.iterable(String.class)) + .contains("isolated:client-a", "isolated:client-b"); } @Test void destroy_clearsAllCachedState() { - factory.httpClient("http://host-a:80", timeouts()); - factory.httpClient("isolated:special", timeouts()); + factory.httpClient("a", config("http://host-a:80", 2000, 2000, 2000)); + RavenApiClientProperties.ApiClientConfigProperties iso = config("http://shared:80"); + iso.setIsolatePool(true); + factory.httpClient("special", iso); factory.destroy(); Assertions.assertThat(factory) - .extracting("baseClients", org.assertj.core.api.InstanceOfAssertFactories.MAP).isEmpty(); + .extracting("baseClients", InstanceOfAssertFactories.MAP).isEmpty(); Assertions.assertThat(factory) - .extracting("ownedProviders", org.assertj.core.api.InstanceOfAssertFactories.MAP).isEmpty(); + .extracting("ownedProviders", InstanceOfAssertFactories.MAP).isEmpty(); } } From b1442daad8c335cb1333085a9e9ba54d9583c530 Mon Sep 17 00:00:00 2001 From: richard483 Date: Sun, 3 May 2026 01:47:37 +0700 Subject: [PATCH 3/4] fix(http): address PR #21 second-round review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses ten new review comments on commit 1214d91: #8/#13 (Copilot) — connectTimeout in the default pool key fragmented pools unnecessarily. CONNECT_TIMEOUT_MILLIS only affects opening new sockets and is reapplied per call as a channel option, so it never persists on a pooled channel; only read/write timeouts (which install channel-bound handlers) need to gate sharing. Default key shape is now scheme://host:port|r=,w=; clients differing only in connect timeout now share a pool. Test updated. #9 (Copilot) — PoolKeys.fromUrl fell back to "raw:" for the "localhost:8080" / "localhost:8080/api" forms used in the setup docs, because URI does not populate host for opaque inputs. Now auto-prepends http:// when there is no scheme so doc-following users actually share a pool. Tests added for both shapes. #10 (Copilot) — Boolean isolatePool's inheritance through PropertiesHelper had no test. Added PropertiesHelperTest covering: default → named inheritance, named explicit override of default, both-unset → null (callers treat as false), and that other optional fields follow the same pattern. #11/#12/#15/#16 (Copilot) — Javadoc and setup docs overstated isolation guarantees. The default factory only isolates the ConnectionProvider; event-loop threads come from Reactor Netty's process-global LoopResources and are still shared. Likewise the shared-pool tunables apply to every ConnectionProvider the default factory creates (including isolated ones), not only shared pools. Wording updated in RavenApiClientProperties.isolatePool, RavenSharedPoolProperties, and docs/setup/index.md to match what the code actually does. #17 (CodeRabbit) — destroy() blocked sequentially with a per-provider 30s timeout, so an app with many pool keys could spend 30s × providerCount in shutdown. Now disposes all providers in parallel via Mono.when, with one overall 30s budget; per-provider failures and timeouts are logged but do not gate the others. #14 (Copilot) — The factory wiring through the interceptor had no integration test. Added RavenHttpClientFactoryWiringTests which boots the full app and verifies that the eight @RavenApiClient interfaces declared in the test resources collapse onto exactly two cached HttpClient base instances (one per backend host:port) and two owned ConnectionProviders — proving (a) the interceptor delegates to the factory and (b) the keying strategy actually shares pools across siblings. Co-Authored-By: Claude Opus 4.7 --- docs/setup/index.md | 15 ++-- .../http/DefaultRavenHttpClientFactory.java | 31 ++++++-- .../raven/apiclient/http/PoolKeys.java | 14 +++- .../http/RavenHttpClientFactory.java | 21 +++--- .../properties/RavenApiClientProperties.java | 18 +++-- .../properties/RavenSharedPoolProperties.java | 11 +-- .../RavenHttpClientFactoryWiringTests.java | 60 +++++++++++++++ .../DefaultRavenHttpClientFactoryTest.java | 51 +++++++++++-- .../apiclient/unit/PropertiesHelperTest.java | 75 +++++++++++++++++++ 9 files changed, 255 insertions(+), 41 deletions(-) create mode 100644 src/test/java/com/nephren/raven/apiclient/RavenHttpClientFactoryWiringTests.java create mode 100644 src/test/java/com/nephren/raven/apiclient/unit/PropertiesHelperTest.java diff --git a/docs/setup/index.md b/docs/setup/index.md index 9e64939..0ae0025 100644 --- a/docs/setup/index.md +++ b/docs/setup/index.md @@ -37,16 +37,19 @@ nephren.raven.apiclient.configs..error-resolver=com.nephren.rave serviceExample.client.errorresolver.DefaultErrorResolver # not required, opt this client out of shared connection pooling so it gets its own -# dedicated Reactor Netty HttpClient + ConnectionProvider, default is false. By default -# clients sharing a scheme://host:port (and identical timeouts) reuse the same pool. +# dedicated Reactor Netty ConnectionProvider, default is false. By default clients +# sharing a scheme://host:port (and identical read/write timeouts) reuse the same pool. +# Note: this isolates the connection pool only — Reactor Netty's process-global event-loop +# threads are still shared. Replace the RavenHttpClientFactory bean if full thread +# isolation is required. nephren.raven.apiclient.configs..isolate-pool=true ``` -## Shared Connection Pool Tunables +## Connection Pool Tunables -These apply to every shared `HttpClient` pool the default factory creates. Most apps will -never need to touch them — defaults match Reactor Netty conventions and are sized for -typical client fan-out. +These apply to every `ConnectionProvider` the default factory creates — both shared pools +and isolated ones. Most apps will never need to touch them; defaults match Reactor Netty +conventions and are sized for typical client fan-out. ``` # not required, max concurrent connections per pool, default is 500 diff --git a/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java b/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java index 4fa7bf1..c71c41d 100644 --- a/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java +++ b/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java @@ -6,11 +6,13 @@ import io.netty.handler.timeout.ReadTimeoutHandler; import io.netty.handler.timeout.WriteTimeoutHandler; import java.time.Duration; +import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.DisposableBean; +import reactor.core.publisher.Mono; import reactor.netty.http.client.HttpClient; import reactor.netty.resources.ConnectionProvider; @@ -91,15 +93,30 @@ private static String sanitize(String poolKey) { .replace("=", "-"); } + /** + * Disposes every cached {@link ConnectionProvider} in parallel and waits at most + * {@link #DISPOSE_TIMEOUT} for the whole batch to complete. Per-provider failures (or a + * provider that exceeds the budget) are logged but do not block the others — disposal of + * one stalled provider must not gate shutdown of the rest of the application. + */ @Override public void destroy() { - ownedProviders.forEach((key, provider) -> { - try { - provider.disposeLater().block(DISPOSE_TIMEOUT); - } catch (RuntimeException e) { - log.warn("#RavenHttpClientFactory failed to dispose connection provider for {}", key, e); - } - }); + if (ownedProviders.isEmpty()) { + return; + } + List> disposals = ownedProviders.entrySet().stream() + .map(entry -> entry.getValue().disposeLater() + .doOnError(err -> log.warn( + "#RavenHttpClientFactory failed to dispose connection provider for {}", + entry.getKey(), err)) + .onErrorResume(err -> Mono.empty())) + .toList(); + try { + Mono.when(disposals).block(DISPOSE_TIMEOUT); + } catch (RuntimeException e) { + log.warn("#RavenHttpClientFactory disposal exceeded {} budget; some providers" + + " may not have shut down cleanly", DISPOSE_TIMEOUT, e); + } ownedProviders.clear(); baseClients.clear(); } diff --git a/src/main/java/com/nephren/raven/apiclient/http/PoolKeys.java b/src/main/java/com/nephren/raven/apiclient/http/PoolKeys.java index c40eae3..47790a4 100644 --- a/src/main/java/com/nephren/raven/apiclient/http/PoolKeys.java +++ b/src/main/java/com/nephren/raven/apiclient/http/PoolKeys.java @@ -8,8 +8,11 @@ * *

The key shape is intentionally narrow — scheme + host + (resolved) port — so that two * clients hitting the same logical backend share a pool regardless of differences in path, - * query, or trailing slashes. Anything we cannot parse falls back to the raw input string, - * which preserves backwards-compatible isolation for malformed URLs without throwing.

+ * query, or trailing slashes. Inputs without an explicit scheme (e.g. the + * {@code localhost:8080} format used in our setup docs) are normalized as if they were + * {@code http://...} so they collapse onto the same key as their scheme-prefixed siblings. + * Anything still unparseable falls back to a {@code raw:} prefix to preserve isolation + * without throwing.

*/ final class PoolKeys { @@ -21,10 +24,15 @@ static String fromUrl(String url) { } try { URI uri = new URI(url); + if (uri.getHost() == null && !url.contains("://")) { + // Inputs like "localhost:8080" or "localhost:8080/api" parse as opaque URIs with no + // host; re-parse with an http:// prefix so they normalize to the same scheme/host/port + // shape as fully-qualified URLs. + uri = new URI("http://" + url); + } String scheme = uri.getScheme() == null ? "http" : uri.getScheme().toLowerCase(); String host = uri.getHost(); if (host == null) { - // not an absolute URL (e.g. just "localhost"); use the raw value as the key return "raw:" + url; } int port = uri.getPort(); diff --git a/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java b/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java index 8ff2301..41b58af 100644 --- a/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java +++ b/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java @@ -38,23 +38,26 @@ public interface RavenHttpClientFactory { HttpClient httpClient(String clientName, ApiClientConfigProperties config); /** - * The default pool key the bundled implementation uses: {@code scheme://host:port|} - * for shared pools, or {@code "isolated:" + clientName} when the client is configured with + * The default pool key the bundled implementation uses: + * {@code scheme://host:port|r=,w=} for shared pools, or + * {@code "isolated:" + clientName} when the client is configured with * {@code isolate-pool: true}. * - *

Timeouts are folded into the shared key because Reactor Netty's - * {@code ReadTimeoutHandler} / {@code WriteTimeoutHandler} attach to the channel, so two - * clients with different read/write timeouts cannot safely share a pooled channel. Custom - * factories that apply timeouts at the request level (e.g. via {@code responseTimeout}) - * may use a narrower key.

+ *

Read and write timeouts are folded into the shared key because Reactor Netty's + * {@link io.netty.handler.timeout.ReadTimeoutHandler} / + * {@link io.netty.handler.timeout.WriteTimeoutHandler} attach to the channel, so two + * clients with different read/write timeouts cannot safely share a pooled channel. Connect + * timeout is intentionally not part of the key — it only affects opening new + * sockets and is reapplied per call as a channel option, so clients that differ only in + * connect timeout can still share a pool. Custom factories that apply timeouts at the + * request level (e.g. via {@code responseTimeout}) may use a narrower key.

*/ static String defaultPoolKey(String clientName, ApiClientConfigProperties config) { if (Boolean.TRUE.equals(config.getIsolatePool())) { return "isolated:" + clientName; } return PoolKeys.fromUrl(config.getUrl()) - + "|c=" + config.getConnectTimeout().toMillis() - + ",r=" + config.getReadTimeout().toMillis() + + "|r=" + config.getReadTimeout().toMillis() + ",w=" + config.getWriteTimeout().toMillis(); } } diff --git a/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java b/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java index a6311ce..2488586 100644 --- a/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java +++ b/src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java @@ -51,12 +51,18 @@ public static class ApiClientConfigProperties { private Class errorResolver; /** - * When {@code true}, this client gets its own Reactor Netty {@code HttpClient} (and hence - * its own connection pool and event loops) instead of sharing the pool keyed by - * scheme+host:port. Treated as {@code false} when unset (most callers benefit from - * sharing). Set to {@code true} when this client has materially different TLS/proxy - * needs from other clients targeting the same host, or when its load profile would - * starve them. + * When {@code true}, this client uses its own dedicated Reactor Netty + * {@code ConnectionProvider} instead of one keyed by scheme+host:port (and read/write + * timeouts). Treated as {@code false} when unset — most callers benefit from sharing. + * Set to {@code true} when this client has TLS/proxy needs that differ from siblings + * targeting the same host, or when its load profile would otherwise starve them on the + * shared connection pool. + * + *

Scope: the default factory only isolates the + * {@code ConnectionProvider} (i.e. the connection pool); event-loop threads come from + * Reactor Netty's process-global {@code LoopResources} and are still shared across all + * clients in the JVM. A custom {@link com.nephren.raven.apiclient.http.RavenHttpClientFactory} + * implementation can replace that if full thread isolation is required.

* *

Modeled as the wrapper {@link Boolean} so that a value set on * {@code configs.default.isolate-pool} can be inherited by named configs that omit the diff --git a/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java b/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java index a357d00..fc05f2a 100644 --- a/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java +++ b/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java @@ -6,13 +6,14 @@ import org.springframework.boot.context.properties.ConfigurationProperties; /** - * Tunables for the Reactor Netty {@code ConnectionProvider} backing every shared - * {@code HttpClient} pool. Most apps will never need to touch these — they are exposed + * Tunables for the Reactor Netty {@code ConnectionProvider} backing every {@code HttpClient} + * the default factory creates. Most apps will never need to touch these — they are exposed * primarily for environments with high client fan-out or unusually slow downstreams. * - *

These values apply to every shared pool the factory creates (one per scheme+host:port - * key). Per-client opt-out via {@code isolate-pool} is configured on each entry in - * {@code nephren.raven.apiclient.configs.}. + *

These values apply to every provider the default factory builds, both shared + * pools (one per scheme+host:port key plus read/write timeouts) and isolated pools opted + * into via {@code configs..isolate-pool: true}. Per-client opt-out controls only + * which key a client lands on — it does not change the per-pool sizing.

*/ @Data @AllArgsConstructor diff --git a/src/test/java/com/nephren/raven/apiclient/RavenHttpClientFactoryWiringTests.java b/src/test/java/com/nephren/raven/apiclient/RavenHttpClientFactoryWiringTests.java new file mode 100644 index 0000000..6afbd72 --- /dev/null +++ b/src/test/java/com/nephren/raven/apiclient/RavenHttpClientFactoryWiringTests.java @@ -0,0 +1,60 @@ +package com.nephren.raven.apiclient; + +import com.nephren.raven.apiclient.http.DefaultRavenHttpClientFactory; +import com.nephren.raven.apiclient.http.RavenHttpClientFactory; +import java.util.Map; +import org.assertj.core.api.Assertions; +import org.assertj.core.api.InstanceOfAssertFactories; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.reactive.AutoConfigureWebTestClient; +import org.springframework.boot.test.context.SpringBootTest; + +/** + * Verifies that {@link RavenHttpClientFactory} is actually consulted by every + * {@code @RavenApiClient} interceptor at startup, and that clients targeting the same + * scheme+host:port collapse onto a single shared {@code HttpClient}. + * + *

The test app declares clients on two backends: + *

    + *
  • {@code localhost:8080} — getExampleClient, postExampleClient, putExampleClient, + * patchExampleClient, deleteExampleClient, emptyExampleClient
    • + *
  • {@code localhost:8081} — exampleClientWithFallback, exampleClientWithOtherFallback, + * deleteExampleClientError
    • + *
+ * All use the default read/write timeouts (so the same pool key suffix), and none set + * {@code isolate-pool}. The factory should therefore end up with exactly two cached base + * clients and two owned providers — proof that the interceptor delegates to the factory and + * that the keying strategy actually shares pools across siblings.

+ */ +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT) +@AutoConfigureWebTestClient +class RavenHttpClientFactoryWiringTests { + + private final RavenHttpClientFactory factory; + + @Autowired + public RavenHttpClientFactoryWiringTests(RavenHttpClientFactory factory) { + this.factory = factory; + } + + @Test + void interceptorWiring_collapsesClientsByBackendOntoSharedBaseClients() { + Assertions.assertThat(factory).isInstanceOf(DefaultRavenHttpClientFactory.class); + + Map baseClients = (Map) org.springframework.test.util.ReflectionTestUtils + .getField(factory, "baseClients"); + Map ownedProviders = (Map) org.springframework.test.util.ReflectionTestUtils + .getField(factory, "ownedProviders"); + + // Two distinct backends → exactly two pool keys, regardless of how many + // @RavenApiClient interfaces target each one. + Assertions.assertThat(baseClients).hasSize(2); + Assertions.assertThat(ownedProviders).hasSize(2); + + Assertions.assertThat(baseClients.keySet()) + .asInstanceOf(InstanceOfAssertFactories.iterable(Object.class)) + .allSatisfy(key -> Assertions.assertThat(key.toString()) + .matches("http://localhost:(8080|8081)\\|r=\\d+,w=\\d+")); + } +} diff --git a/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java b/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java index 82a758f..eca5861 100644 --- a/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java +++ b/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java @@ -42,10 +42,20 @@ private static RavenApiClientProperties.ApiClientConfigProperties config( } @Test - void defaultPoolKey_byDefault_usesSchemeHostPortPlusTimeoutFingerprint() { + void defaultPoolKey_byDefault_usesSchemeHostPortPlusReadWriteTimeouts() { String key = RavenHttpClientFactory.defaultPoolKey( "any-name", config("http://example.com:8080/api", 1000, 2000, 3000)); - Assertions.assertThat(key).isEqualTo("http://example.com:8080|c=1000,r=2000,w=3000"); + Assertions.assertThat(key).isEqualTo("http://example.com:8080|r=2000,w=3000"); + } + + @Test + void defaultPoolKey_ignoresConnectTimeout_soClientsCanShareWhenItDiffers() { + // CONNECT_TIMEOUT_MILLIS only affects new socket attempts — it is reapplied per call as + // a channel option and never persists on a pooled channel. Clients that differ only in + // connect timeout must therefore share a pool. + String a = RavenHttpClientFactory.defaultPoolKey("x", config("http://e:80", 1000, 2000, 2000)); + String b = RavenHttpClientFactory.defaultPoolKey("y", config("http://e:80", 9000, 2000, 2000)); + Assertions.assertThat(a).isEqualTo(b); } @Test @@ -58,6 +68,26 @@ void defaultPoolKey_resolvesDefaultPortPerScheme() { .startsWith("https://example.com:443|"); } + @Test + void defaultPoolKey_acceptsBareHostPortUrlFromSetupDocs() { + // The setup docs use "localhost:8080" (no scheme); make sure that normalizes onto the + // same key as "http://localhost:8080" so doc-following users actually share a pool. + String bare = RavenHttpClientFactory.defaultPoolKey( + "a", config("localhost:8080", 2000, 2000, 2000)); + String full = RavenHttpClientFactory.defaultPoolKey( + "b", config("http://localhost:8080", 2000, 2000, 2000)); + Assertions.assertThat(bare) + .isEqualTo(full) + .startsWith("http://localhost:8080|"); + } + + @Test + void defaultPoolKey_acceptsBareHostPortWithPath() { + String key = RavenHttpClientFactory.defaultPoolKey( + "a", config("localhost:8080/api", 2000, 2000, 2000)); + Assertions.assertThat(key).startsWith("http://localhost:8080|"); + } + @Test void defaultPoolKey_whenIsolatePoolSet_returnsIsolatedKeyWithClientName() { RavenApiClientProperties.ApiClientConfigProperties c = config("http://shared-host:8080"); @@ -89,17 +119,28 @@ void httpClient_withDifferentHosts_allocatesDistinctBaseClients() { } @Test - void httpClient_withSameHostButDifferentTimeouts_allocatesDistinctBaseClients() { + void httpClient_withSameHostButDifferentReadOrWriteTimeouts_allocatesDistinctBaseClients() { // Channel-bound ReadTimeoutHandler/WriteTimeoutHandler would otherwise leak across - // clients sharing a pooled channel; the default key folds timeouts in to prevent it. + // clients sharing a pooled channel; the default key folds those timeouts in to prevent + // it. Connect timeout is intentionally excluded (see separate test). factory.httpClient("a", config("http://example.com:80", 2000, 2000, 2000)); - factory.httpClient("b", config("http://example.com:80", 5000, 5000, 5000)); + factory.httpClient("b", config("http://example.com:80", 2000, 5000, 2000)); Assertions.assertThat(factory) .extracting("baseClients", InstanceOfAssertFactories.MAP) .hasSize(2); } + @Test + void httpClient_withSameHostAndReadWriteTimeouts_sharesEvenWhenConnectTimeoutDiffers() { + factory.httpClient("a", config("http://example.com:80", 1000, 2000, 2000)); + factory.httpClient("b", config("http://example.com:80", 9000, 2000, 2000)); + + Assertions.assertThat(factory) + .extracting("baseClients", InstanceOfAssertFactories.MAP) + .hasSize(1); + } + @Test void isolatedClients_eachOwnTheirOwnConnectionProvider() { RavenApiClientProperties.ApiClientConfigProperties a = config("http://shared:80"); diff --git a/src/test/java/com/nephren/raven/apiclient/unit/PropertiesHelperTest.java b/src/test/java/com/nephren/raven/apiclient/unit/PropertiesHelperTest.java new file mode 100644 index 0000000..f9f39cf --- /dev/null +++ b/src/test/java/com/nephren/raven/apiclient/unit/PropertiesHelperTest.java @@ -0,0 +1,75 @@ +package com.nephren.raven.apiclient.unit; + +import com.nephren.raven.apiclient.properties.PropertiesHelper; +import com.nephren.raven.apiclient.properties.RavenApiClientProperties.ApiClientConfigProperties; +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.Test; + +/** + * Locks in the inheritance contract of + * {@link PropertiesHelper#copyConfigPropertiesFromSourceToTarget}: optional fields set on + * {@code configs.default} must flow through to named configs that omit them, but explicit + * values on the named config must still win. + * + *

Specifically guards the {@code isolate-pool} regression Copilot flagged on PR #21 + * comment #4 — modeling the field as primitive {@code boolean} silently overwrote an + * inherited {@code true} back to {@code false} for any named config that did not set the + * key explicitly.

+ */ +class PropertiesHelperTest { + + /** Equivalent to {@code mergeApiClientConfigProperties} in {@code RequestMappingMetadataBuilder}. */ + private static ApiClientConfigProperties merge( + ApiClientConfigProperties defaults, ApiClientConfigProperties named) { + ApiClientConfigProperties merged = new ApiClientConfigProperties(); + PropertiesHelper.copyConfigPropertiesFromSourceToTarget(defaults, merged); + PropertiesHelper.copyConfigPropertiesFromSourceToTarget(named, merged); + return merged; + } + + @Test + void isolatePool_setOnDefault_inheritsToNamedConfigThatOmitsIt() { + ApiClientConfigProperties defaults = new ApiClientConfigProperties(); + defaults.setIsolatePool(true); + ApiClientConfigProperties named = new ApiClientConfigProperties(); + // named omits isolate-pool — the merged value must come from defaults + + ApiClientConfigProperties merged = merge(defaults, named); + + Assertions.assertThat(merged.getIsolatePool()).isTrue(); + } + + @Test + void isolatePool_explicitOnNamedConfig_overridesDefault() { + ApiClientConfigProperties defaults = new ApiClientConfigProperties(); + defaults.setIsolatePool(true); + ApiClientConfigProperties named = new ApiClientConfigProperties(); + named.setIsolatePool(false); + + ApiClientConfigProperties merged = merge(defaults, named); + + Assertions.assertThat(merged.getIsolatePool()).isFalse(); + } + + @Test + void isolatePool_unsetOnBoth_remainsNullSoCallersTreatItAsFalse() { + ApiClientConfigProperties merged = merge( + new ApiClientConfigProperties(), new ApiClientConfigProperties()); + + Assertions.assertThat(merged.getIsolatePool()).isNull(); + } + + @Test + void otherOptionalFields_followTheSameInheritancePattern() { + ApiClientConfigProperties defaults = new ApiClientConfigProperties(); + defaults.setUrl("http://default-host:8080"); + defaults.getHeaders().put("X-Common", "yes"); + ApiClientConfigProperties named = new ApiClientConfigProperties(); + named.setUrl("http://named-host:9090"); + + ApiClientConfigProperties merged = merge(defaults, named); + + Assertions.assertThat(merged.getUrl()).isEqualTo("http://named-host:9090"); + Assertions.assertThat(merged.getHeaders()).containsEntry("X-Common", "yes"); + } +} From 0a63e24b22a9151233498e99ee946a03e90a5de9 Mon Sep 17 00:00:00 2001 From: richard483 Date: Sun, 3 May 2026 02:29:34 +0700 Subject: [PATCH 4/4] fix(http): address PR #21 third-round review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4 (Copilot) — pendingAcquireMaxCount default of -1 was not the Reactor Netty default. Reactor Netty's library default is 2 * maxConnections (bounded); -1 in our config was being passed straight to the builder and produced an unbounded queue, which would amplify a single backend outage into application-wide heap pressure. Per the user's guidance, the value -1 should mean "use Reactor Netty's default", so the factory now skips calling .pendingAcquireMaxCount() when the property is -1 and lets the library compute 2 * maxConnections. Any non-negative value is still applied as-is. Property javadoc and setup docs updated to describe this contract. #1 (CodeRabbit) — Timeout properties were applied with no bounds checks, so a misconfigured null/negative duration would trip a NullPointerException deep in Netty, and a duration that exceeded Integer.MAX_VALUE millis would silently overflow when cast for CONNECT_TIMEOUT_MILLIS. Added validateTimeoutMillis up front with clear messages naming the offending field and client; covered by three new unit tests (null, non-positive, int overflow). #6 (Copilot) — RavenHttpClientFactory javadoc claimed the default implementation shared event loops by pool key. It does not — Reactor Netty's HttpClient.create(provider) reuses the process-global LoopResources regardless of provider. Reworded to say sharing is at the ConnectionProvider level only and that replacing this bean is the way to also isolate event loops. #7 (Copilot) — RavenHttpClientFactoryWiringTests asserted hasSize(2), but emptyExampleClient has no explicit URL config so it falls through to the ApiClientConfigProperties default of "localhost", producing a third pool key. Updated the assertions and javadoc to expect three backends (localhost:80 / 8080 / 8081). #8 (Copilot) — PoolKeys.fromUrl normalized scheme-less inputs like "localhost:8080" for keying, but the interceptor still passed the raw property value into WebClient.Builder.baseUrl(...), so doc-following users would get a malformed base URL at request time. Added RavenHttpClientFactory.normalizedBaseUrl(...) and routed the interceptor through it so the same normalization applies in both places. #3 / #5 (Copilot, CodeRabbit) — Setup docs claimed the connection pool defaults "match Reactor Netty conventions". They don't — the default max-connections=500 is a Raven override above the library default, and pending-acquire-max-count=-1 is a sentinel that delegates to Reactor Netty's actual default (2 * maxConnections). Reworded. #2 (CodeRabbit) — Fenced code blocks in docs/setup/index.md now carry the `properties` language identifier so markdownlint stops flagging MD040 on the new shared-pool block (and the existing ones). Co-Authored-By: Claude Opus 4.7 --- docs/setup/index.md | 20 ++++--- .../aop/RavenApiClientMethodInterceptor.java | 3 +- .../http/DefaultRavenHttpClientFactory.java | 54 +++++++++++++++---- .../http/RavenHttpClientFactory.java | 25 +++++++-- .../properties/RavenSharedPoolProperties.java | 11 ++-- .../RavenHttpClientFactoryWiringTests.java | 25 +++++---- .../DefaultRavenHttpClientFactoryTest.java | 30 +++++++++++ 7 files changed, 130 insertions(+), 38 deletions(-) diff --git a/docs/setup/index.md b/docs/setup/index.md index 0ae0025..c2ea063 100644 --- a/docs/setup/index.md +++ b/docs/setup/index.md @@ -9,7 +9,7 @@ has_children: false ## Api Client Properties -``` +```properties # where your api client interfaces are located for registering api client bean(required) nephren.raven.apiclient.packages=com.example.apiclient @@ -48,21 +48,27 @@ nephren.raven.apiclient.configs..isolate-pool=true ## Connection Pool Tunables These apply to every `ConnectionProvider` the default factory creates — both shared pools -and isolated ones. Most apps will never need to touch them; defaults match Reactor Netty -conventions and are sized for typical client fan-out. +and isolated ones. Most apps will never need to touch them. -``` -# not required, max concurrent connections per pool, default is 500 +The default `max-connections=500` is a Raven override above Reactor Netty's library default +to better fit a typical multi-client fan-out pattern. The default +`pending-acquire-max-count=-1` is a sentinel meaning "use Reactor Netty's library default" +of `2 × max-connections`; set a positive value to override (smaller for fail-fast under +load, larger for very bursty workloads). + +```properties +# not required, max concurrent connections per pool, default is 500 (Raven override) nephren.raven.apiclient.shared-pool.max-connections=500 # not required, max queued acquire attempts when the pool is saturated. -# -1 (default) is unbounded; set a positive value to fail-fast under load. +# -1 (default) means use Reactor Netty's default (2 * max-connections); +# set a positive value to override. nephren.raven.apiclient.shared-pool.pending-acquire-max-count=-1 ``` ## Api Scheduler -``` +```properties # not required, customize scheduler flavors, default is immediate nephren.raven.reactor.helper.configs..type=single diff --git a/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java b/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java index 9dff82d..1c3b44c 100644 --- a/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java +++ b/src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java @@ -91,8 +91,9 @@ private void prepareBodyResolvers() { private void prepareWebClient() { RavenHttpClientFactory factory = applicationContext.getBean(RavenHttpClientFactory.class); HttpClient httpClient = factory.httpClient(name, metadata.getProperties()); + String baseUrl = RavenHttpClientFactory.normalizedBaseUrl(metadata.getProperties().getUrl()); WebClient.Builder builder = applicationContext.getBean(WebClient.Builder.class) - .exchangeStrategies(getExchangeStrategies()).baseUrl(metadata.getProperties().getUrl()) + .exchangeStrategies(getExchangeStrategies()).baseUrl(baseUrl) .clientConnector(new ReactorClientHttpConnector(httpClient)) .defaultHeaders( httpHeaders -> metadata.getProperties().getHeaders().forEach(httpHeaders::add)); diff --git a/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java b/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java index c71c41d..332eb45 100644 --- a/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java +++ b/src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java @@ -52,30 +52,62 @@ public DefaultRavenHttpClientFactory(RavenSharedPoolProperties sharedPoolPropert @Override public HttpClient httpClient(String clientName, ApiClientConfigProperties config) { + int connectMs = validateTimeoutMillis(config.getConnectTimeout(), "connect", clientName); + long readMs = validateTimeoutMillis(config.getReadTimeout(), "read", clientName); + long writeMs = validateTimeoutMillis(config.getWriteTimeout(), "write", clientName); + String poolKey = RavenHttpClientFactory.defaultPoolKey(clientName, config); HttpClient base = baseClients.computeIfAbsent(poolKey, this::buildBase); return base - .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, - (int) config.getConnectTimeout().toMillis()) + .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, connectMs) .doOnConnected(connection -> connection - .addHandlerLast(new ReadTimeoutHandler( - config.getReadTimeout().toMillis(), TimeUnit.MILLISECONDS)) - .addHandlerLast(new WriteTimeoutHandler( - config.getWriteTimeout().toMillis(), TimeUnit.MILLISECONDS))); + .addHandlerLast(new ReadTimeoutHandler(readMs, TimeUnit.MILLISECONDS)) + .addHandlerLast(new WriteTimeoutHandler(writeMs, TimeUnit.MILLISECONDS))); + } + + /** + * Reject null, non-positive, or int-overflowing timeouts up front so a misconfiguration + * surfaces with a clear message instead of silent truncation in Netty. + */ + private static int validateTimeoutMillis(java.time.Duration value, String name, + String clientName) { + if (value == null) { + throw new IllegalArgumentException( + "#RavenHttpClientFactory " + name + "Timeout must not be null for client '" + + clientName + "'"); + } + long millis = value.toMillis(); + if (millis <= 0) { + throw new IllegalArgumentException( + "#RavenHttpClientFactory " + name + "Timeout must be > 0ms for client '" + + clientName + "', got " + millis + "ms"); + } + if (millis > Integer.MAX_VALUE) { + throw new IllegalArgumentException( + "#RavenHttpClientFactory " + name + "Timeout exceeds the supported range for" + + " client '" + clientName + "': " + millis + "ms (max " + + Integer.MAX_VALUE + "ms)"); + } + return (int) millis; } private HttpClient buildBase(String poolKey) { - ConnectionProvider provider = ConnectionProvider.builder("raven-" + sanitize(poolKey)) - .maxConnections(sharedPoolProperties.getMaxConnections()) - .pendingAcquireMaxCount(sharedPoolProperties.getPendingAcquireMaxCount()) - .build(); + ConnectionProvider.Builder builder = ConnectionProvider.builder("raven-" + sanitize(poolKey)) + .maxConnections(sharedPoolProperties.getMaxConnections()); + int pendingMax = sharedPoolProperties.getPendingAcquireMaxCount(); + // -1 sentinel → leave the builder unset so Reactor Netty's library default + // (2 * maxConnections) applies. Any other value is a deliberate override. + if (pendingMax >= 0) { + builder.pendingAcquireMaxCount(pendingMax); + } + ConnectionProvider provider = builder.build(); ownedProviders.put(poolKey, provider); log.debug( "#RavenHttpClientFactory creating HttpClient for {} (maxConnections={}," + " pendingAcquireMaxCount={})", poolKey, sharedPoolProperties.getMaxConnections(), - sharedPoolProperties.getPendingAcquireMaxCount()); + pendingMax < 0 ? "default(2*maxConnections)" : pendingMax); return HttpClient.create(provider); } diff --git a/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java b/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java index 41b58af..41189c8 100644 --- a/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java +++ b/src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java @@ -7,11 +7,12 @@ * Extension point for materializing the Reactor Netty {@link HttpClient} that backs each * {@code @RavenApiClient}'s {@code WebClient}. * - *

The default implementation shares connection pools and event loops across all clients - * targeting the same scheme+host:port (with the same configured timeouts) and applies the - * per-client connect/read/write timeouts on top. Replace this bean to take full control of - * pooling — typical reasons include custom TLS configuration, a metric-instrumented - * {@code ConnectionProvider}, sharing pools with other parts of an application, or a + *

The default implementation shares connection pools across clients targeting the same + * scheme+host:port with the same read/write timeouts, and applies per-client + * connect/read/write timeouts on top. Reactor Netty's event-loop threads + * ({@code LoopResources}) come from the process-global instance regardless of pool key, so + * isolation here is at the {@code ConnectionProvider} level only — replace this bean to + * also isolate event loops, customize TLS, instrument the pool with metrics, or use a * different keying strategy.

* *

The full per-client configuration is passed to {@link #httpClient(String, @@ -60,4 +61,18 @@ static String defaultPoolKey(String clientName, ApiClientConfigProperties config + "|r=" + config.getReadTimeout().toMillis() + ",w=" + config.getWriteTimeout().toMillis(); } + + /** + * Returns a base-URL string suitable for {@code WebClient.Builder.baseUrl(...)} — the + * configured value as-is when it already carries a scheme, otherwise prefixed with + * {@code http://} so the documented {@code localhost:8080} format works at request time + * the same way it works for pool-key derivation. Returns the input unchanged when null + * or blank so existing validation/error paths still see the original value. + */ + static String normalizedBaseUrl(String url) { + if (url == null || url.isBlank()) { + return url; + } + return url.contains("://") ? url : "http://" + url; + } } diff --git a/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java b/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java index fc05f2a..df1a790 100644 --- a/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java +++ b/src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java @@ -29,9 +29,14 @@ public class RavenSharedPoolProperties { private int maxConnections = 500; /** - * Maximum number of acquire attempts allowed to queue while the pool is saturated. {@code -1} - * means unbounded (Reactor Netty default). Set a positive value to fail-fast under load - * instead of letting backpressure pile up. + * Maximum number of acquire attempts allowed to queue while the pool is saturated. The + * default of {@code -1} is a sentinel meaning "use Reactor Netty's library default", which + * is {@code 2 * maxConnections}. Set a positive value to override (e.g. fail-fast under + * load with a small queue, or raise the ceiling for very bursty workloads). + * + *

The library default is intentionally bounded — never set this to a value that would + * let a single wedged downstream accumulate unbounded queued acquires and amplify a + * single-backend outage into application-wide heap pressure.

*/ private int pendingAcquireMaxCount = -1; diff --git a/src/test/java/com/nephren/raven/apiclient/RavenHttpClientFactoryWiringTests.java b/src/test/java/com/nephren/raven/apiclient/RavenHttpClientFactoryWiringTests.java index 6afbd72..92d42d4 100644 --- a/src/test/java/com/nephren/raven/apiclient/RavenHttpClientFactoryWiringTests.java +++ b/src/test/java/com/nephren/raven/apiclient/RavenHttpClientFactoryWiringTests.java @@ -15,17 +15,20 @@ * {@code @RavenApiClient} interceptor at startup, and that clients targeting the same * scheme+host:port collapse onto a single shared {@code HttpClient}. * - *

The test app declares clients on two backends: + *

The test app declares clients on three distinct backends (after default-config + * resolution): *

    *
  • {@code localhost:8080} — getExampleClient, postExampleClient, putExampleClient, - * patchExampleClient, deleteExampleClient, emptyExampleClient
    • + * patchExampleClient, deleteExampleClient (5 clients) *
  • {@code localhost:8081} — exampleClientWithFallback, exampleClientWithOtherFallback, - * deleteExampleClientError
    • + * deleteExampleClientError (3 clients) + *
  • {@code localhost:80} — emptyExampleClient (no explicit URL, falls through to the + * {@code ApiClientConfigProperties} default of {@code "localhost"}, 1 client)
    • *
- * All use the default read/write timeouts (so the same pool key suffix), and none set - * {@code isolate-pool}. The factory should therefore end up with exactly two cached base - * clients and two owned providers — proof that the interceptor delegates to the factory and - * that the keying strategy actually shares pools across siblings.

+ * All use the default read/write timeouts and none set {@code isolate-pool}, so the + * factory should end up with exactly three cached base clients and three owned providers + * — proof that the interceptor delegates to the factory and that the keying strategy + * actually shares pools across siblings on the same backend.

*/ @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT) @AutoConfigureWebTestClient @@ -47,14 +50,14 @@ void interceptorWiring_collapsesClientsByBackendOntoSharedBaseClients() { Map ownedProviders = (Map) org.springframework.test.util.ReflectionTestUtils .getField(factory, "ownedProviders"); - // Two distinct backends → exactly two pool keys, regardless of how many + // Three distinct backends → exactly three pool keys, regardless of how many // @RavenApiClient interfaces target each one. - Assertions.assertThat(baseClients).hasSize(2); - Assertions.assertThat(ownedProviders).hasSize(2); + Assertions.assertThat(baseClients).hasSize(3); + Assertions.assertThat(ownedProviders).hasSize(3); Assertions.assertThat(baseClients.keySet()) .asInstanceOf(InstanceOfAssertFactories.iterable(Object.class)) .allSatisfy(key -> Assertions.assertThat(key.toString()) - .matches("http://localhost:(8080|8081)\\|r=\\d+,w=\\d+")); + .matches("http://localhost:(80|8080|8081)\\|r=\\d+,w=\\d+")); } } diff --git a/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java b/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java index eca5861..3b26cdc 100644 --- a/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java +++ b/src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java @@ -160,6 +160,36 @@ void isolatedClients_eachOwnTheirOwnConnectionProvider() { .contains("isolated:client-a", "isolated:client-b"); } + @Test + void httpClient_rejectsNullReadTimeout() { + RavenApiClientProperties.ApiClientConfigProperties bad = config("http://example.com"); + bad.setReadTimeout(null); + + Assertions.assertThatThrownBy(() -> factory.httpClient("client", bad)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("readTimeout").hasMessageContaining("null"); + } + + @Test + void httpClient_rejectsNonPositiveTimeout() { + RavenApiClientProperties.ApiClientConfigProperties bad = + config("http://example.com", 2000, 0, 2000); + + Assertions.assertThatThrownBy(() -> factory.httpClient("client", bad)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("readTimeout").hasMessageContaining("> 0ms"); + } + + @Test + void httpClient_rejectsTimeoutExceedingIntRange() { + RavenApiClientProperties.ApiClientConfigProperties bad = config("http://example.com"); + bad.setConnectTimeout(Duration.ofMillis(((long) Integer.MAX_VALUE) + 1)); + + Assertions.assertThatThrownBy(() -> factory.httpClient("client", bad)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("connectTimeout").hasMessageContaining("supported range"); + } + @Test void destroy_clearsAllCachedState() { factory.httpClient("a", config("http://host-a:80", 2000, 2000, 2000));