ReactorClientHttpConnector creates new HttpClient for every request

[kzander91]

In 6.1.9, since commit 7785f94 (related issue: #32945), this.httpClient is no longer assigned. The assignment should probably have happened here:

spring-framework/spring-web/src/main/java/org/springframework/http/client/reactive/ReactorClientHttpConnector.java

Lines 128 to 131 in 5356a1b

	if (httpClient == null) {
	Assert.state(this.resourceFactory != null && this.mapper != null, "Illegal configuration");
	httpClient = createHttpClient(this.resourceFactory, this.mapper);
	}

As a consequence, a new HttpClient instance is created for every request. Apart from the possible performance impact, this is causing a memory leak in my application: I'm using client certificate authentication configured through Boot's SslBundle support. This causes the creation of new SslContext instances alongside each new HttpClient in org.springframework.boot.autoconfigure.web.reactive.function.client.ReactorClientHttpConnectorFactory.
Then, because Netty's SslContext implementations don't implement hashCode(), Reactor Netty's connection pooling doesn't work properly, causing new HTTP connection pools to be created for every request (config.channelHash() returns distinct values for every request https://github.com/reactor/reactor-netty/blob/v1.1.20/reactor-netty-core/src/main/java/reactor/netty/resources/PooledConnectionProvider.java#L127-L133).
All these connection pools are then never disposed of (this is Reactor Netty's default configuration I believe), eventually causing an OOME.

---

TL;DR

• Reactor Netty creates a new HTTP connection pool for every HTTP request, because
• it includes the hash code of Netty's SslContext in the pool key, because
• an SslContext has been configured by Spring Boot's SslBundle feature for every request, because
• Spring Framework creates new HttpClient instances for every request.

The leak itself may be fixed in Netty (should SslContext implementations have stable hash codes?), but it was surfaced by the commit in Spring Framework.

[kzander91]

Further insight: It looks like this happens to all ReactorClientHttpConnector instances that are not Spring Beans and that are created through this constructor at a time when resourceFactory.isRunning() returns false:

spring-framework/spring-web/src/main/java/org/springframework/http/client/reactive/ReactorClientHttpConnector.java

Lines 109 to 115 in 06b492d

	public ReactorClientHttpConnector(ReactorResourceFactory resourceFactory, Function<HttpClient, HttpClient> mapper) {
	this.resourceFactory = resourceFactory;
	this.mapper = mapper;
	if (resourceFactory.isRunning()) {
	this.httpClient = createHttpClient(resourceFactory, mapper);
	}
	}

For those instances, the Lifecycle.start() method isn't called, and the httpClient stays null.

[jhoeller]

Where does the ReactorResourceFactory instance come from in your scenario? It seems to not have been initialized/started at the time of ReactorClientHttpConnector construction yet, so at which point will it receive an afterPropertiesSet() or start() call then? Is the ReactorResourceFactory possibly a Spring bean but ReactorClientHttpConnector is not? If this is the case, we could simply retain the HttpClient instance in connect if the ReactorResourceFactory has been started in the meantime.

[kzander91]

@jhoeller I'm doing this:

@Bean
WebClient apiV1WebClient(WebClient.Builder builder, WebClientSsl ssl) {
    return builder
            .apply(ssl.fromBundle("client-auth"))
            .build();
}

The ReactorResourceFactory ultimately comes from org.springframework.boot.autoconfigure.reactor.netty.ReactorNettyConfigurations.ReactorResourceFactoryConfiguration. So yes, it is a bean, but the ReactorClientHttpConnector is not. It is created and passed into the web client builder in the apply() call, see https://github.com/spring-projects/spring-boot/blob/586499db56afc755a1e5e623afc5bb636c601562/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/reactive/function/client/AutoConfiguredWebClientSsl.java#L48-L53.

[jhoeller]

Alright, thanks for the deep dive! I'll fix this for 6.1.11 as suggested above then since it matches the scenario I had in mind there.