spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
55.6k stars 37.74k forks source link

ReactorClientHttpConnector creates new HttpClient for every request #33093

Closed kzander91 closed 2 days ago

kzander91 commented 4 days ago

In 6.1.9, since commit https://github.com/spring-projects/spring-framework/commit/7785f94c4c2b7a35d80c28f197c87c54fffc6564 (related issue: #32945), this.httpClient is no longer assigned. The assignment should probably have happened here: https://github.com/spring-projects/spring-framework/blob/5356a1b1ac983c1e121d809fb54c3cb43f640f9b/spring-web/src/main/java/org/springframework/http/client/reactive/ReactorClientHttpConnector.java#L128-L131

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

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 commented 4 days ago

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: https://github.com/spring-projects/spring-framework/blob/06b492dc0e8daa296ba0428a150289a6c4fd5cfd/spring-web/src/main/java/org/springframework/http/client/reactive/ReactorClientHttpConnector.java#L109-L115

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

jhoeller commented 4 days ago

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 commented 4 days ago

@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 commented 4 days ago

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.