Make it easier to tidy up the resources used by a WebClient [SPR-16963]

[spring-projects-issues]

Andy Wilkinson opened SPR-16963 and commented

It's really easy to create a WebClient using its builder but this can trigger the creation of a number of threads that can't then be cleaned up when the client's no longer needed.

I'd like to keep the easy creation but also be able to close it so that I'm left with the same number of threads running as there were before the client was created. If WebClient is to replace RestTemplate, I think this'll be of increased importance as people may want to use it as a library outside of an application context.

---

Affects: 5.0.7

Issue Links:

• Add JettyResourceFactory [SPR-17179] #21715 Add JettyResourceFactory

Referenced from: commits 7a0c03e, 1bc08c6

[spring-projects-issues]

Rossen Stoyanchev commented

For some background, by default with Rector Netty, the WebClient does nothing specific for managing resources, and intentionally so, relying on global resources from Reactor Netty that in turn are shared across any number of client and server instances. In a non-blocking, reactive stack where we only want one event loop, not many, this is a good default behavior. By contrast, to automatically create resources by default puts the burden on the developer to do the right thing by sharing the resources across client and server.

In a non-blocking stack, it still makes sense to share resources across any number of client instances, so this is arguably still the right thing to do out of the box.

Nevertheless it is a valid question for what is the right way to clean up those resources when necessary? Perhaps we should consider how to make it easy to integrate the Reactor Netty global resources into the lifecycle of a Spring application (e.g. Lifecycle calling HttpResources.shutdown()), so they can be started and easily cleaned up?

[spring-projects-issues]

Rossen Stoyanchev commented

Assigning tentatively for 5.1.

[spring-projects-issues]

Rossen Stoyanchev commented

[1] Request to make HttpClient implement Disposable in reactor-netty#88.
[2] Devtools restart and Reactor Netty resource issue in spring-boot#9146.
[3] Somewhat related, graceful shutdown in spring-boot#4657.

Overall, as the conclusion in [1] goes, it would be somewhat dangerous and misleading to expose anything via WebClient, because most likely those are global resources, possibly and likely shared with the server. So it's more about finding right place to shut them first and then what API should be used (Reactor Netty specific, WeFlux, etc) [2].

The answer may also vary by HTTP client, e.g. we're adding Jetty support for WebClient, and that has a different usage model.

[spring-projects-issues]

Rossen Stoyanchev commented

To answer the original question, with Reactor Netty, these are the two options:

1. Create WebClient with no special configuration related to managing resources (i.e. participating in global Reactor Netty resources) and then call HttpResources.shutdown() when done.

2. Create WebClient with custom setup to create your own resources and then shut them down.

LoopResources resources = LoopResources.create("test-loop");
ConnectionProvider provider = ConnectionProvider.elastic("test-pool");

TcpClient tcpClient = TcpClient.create(provider).runOn(resources, false);
HttpClient httpClient = HttpClient.from(tcpClient);

WebClient webClient = WebClient.builder()
	.clientConnector(new ReactorClientHttpConnector(httpClient))
	.build();

// Use WebClient...

provider.dispose();
resources.dispose();

The above should probably be documented somewhere, since it wouldn't be easy to discover on your own. We could also think about possible ways at the level of Reactor Netty APIs to make the above easier to work with. That said my sense is that use of global resources is the most common option, and in any case for the given scenario, option 1) is the best answer.

Also I don't think it makes sense to wrap the logic of creating and shutting down HTTP client resources through some common abstraction in Spring APIs.

Andy Wilkinson what do you think?

[spring-projects-issues]

Andy Wilkinson commented

The problem that originally motivated this issue was that I had observed in Boot's tests suite that numerous WebClient-related threads were left hanging around after the tests that use WebClient had finished and their application contexts had been closed. As I alluded to in my opening comment, there's also a broader problem here in other environments. For example, if WebClient is used in a war deployed to Tomcat, Tomcat will complain when the app is undeployed as it leaves threads running. That will apply both if WebClient is used as a bean managed in an application context or purely as a library.

In these cases relying on HttpResources.shutdown() to tidy things up doesn't feel right as there's an asymmetry between the resources being created implicitly but needing to be shut down explicitly. There's also the complexity off knowing that global shared resources are being used and understanding when it's safe to shut them down.

Explicitly creating the resources and then explicitly shutting them down makes things symmetrical but it feels like it's placing a greater burden on users than ought to be necessary. I guess that's where an API in Reactor Netty to make this easier to work with would come in.

In my opinion, the default and most common option with WebClient should be that it uses separate resources that can be tidied up by calling a close() method on the client. If global resources are to be used, that should be something that the user opts into. Making users opt in would make them aware of the global resources in the first place, and would also hopefully make them consider the complexity of figuring out when they should be cleaned up.

[spring-projects-issues]

Rossen Stoyanchev commented

It's a bit late to change the default at this point. That aside it all sounds reasonable for thread-pool based concurrency. Yet I'm not sure which is better for event loop concurrency.

Reactor Netty can be embedded in a number of places besides the WebClient such as the Spring Cloud Gateway or the Java CF client to name two. Is it better to put the complexity in knowing they should share resources, and in making sure that's the case? Spring Boot could automatically take care of WebFlux client + server, Spring Cloud Gateway could partake too, but it might be more of a challenge for Java CF client and others that don't build on Boot to participate in which case it's left as an exercise?

It's either run with sub-optimal settings, possibly not even realizing it, or have a fixed number of Reactor Netty threads that may be left hanging. But it's also worth considering when does that actually become an issue? For example in a WebFlux application it's typically not ever an issue, so it's mostly a benefit. A Spring MVC app however can be in a WAR, in which case it would make sense to shut down global Reactor Netty resources whenever the ServletContetx closes, which can be done in a ServletContextInitializer (like the one from Boot). It still feels like a reasonably straight-forward thing to do, to have to shut down global resources when necessary, especially if it's documented and easy, compared to the alternative of ensuring resource sharing across components and libraries. 

Keep in mind the RestTemplate also does not have a close method. Instead individual ClientHttpRequestFactory's implement InitializingBean/DispoableBean.  We can enhance ReactorHttpClientConnector along similar lines but in practice that'd be helpful in tests primarily (where it's just as easy to call HttpResources#dispose) since in most cases I'd expect a WebClient to not manage its own resources but to participate in shared ones. 

 

[spring-projects-issues]

Rossen Stoyanchev commented

I have one concrete suggestion for an HttpResourcesFactory that would manage Reactor Netty resources within a Spring ApplicationContext. It can be created in global mode, i.e. managing the global holder HttpResources, or otherwise independently manage a ConnectionrProvider / LoopResources pair, much like HttpResources does.

We would also allow ReactorClientHttpConnector to be injected with such a factory, so it can be used to configure the TcpClient with the given resources, that are managed independently. Or with the global mode, all you need to do is declare such a factory in Spring configuration and it would stop with the ApplicationContext.

The global mode is a bit like configuring a JSR-356 WebSocketContainer by declaring a WebSocketContainerFactoryBean, and not ever injecting it into anything (it's statically accessible). I've also like Java's ForkJoinPool#commonPool() and Spring's ForkJoinPoolFactoryBean that let's you manage the common pool.

 

 

[spring-projects-issues]

Rossen Stoyanchev commented

I've added ReactorResourceFactory with a globalResources flag set to "true" by default. So I'd expect in most cases it would simply be declared as a bean in which case it helps to initialize and shut down the global resources.

I've also  added a constructor to ReactorClientHttpConnector that accepts the factory the can be used with a factory where globaResources=false but I'd expect that to be the less common case.

[spring-projects-issues]

Andy Wilkinson commented

Thank you! I've opened spring-projects/spring-boot#14058 to make use of the resource factory in Boot.

[spring-projects-issues]

Rossen Stoyanchev commented

Docs updated with advice on managing resources.

I've also created #21715 and will add a similar section for the new Jetty HttpClient integration.

[quaff]

I'm trying spring 5.2.0.RC1 and reactor-3.2.11 and reactor-netty-0.8.10, failed to clean up resources.

	@Bean
	public ReactorResourceFactory resourceFactory() {
		ReactorResourceFactory factory = new ReactorResourceFactory();
		factory.setUseGlobalResources(false);
		return factory;
	}

	@Bean
	public WebClient webClient() {
		ClientHttpConnector connector = new ReactorClientHttpConnector(resourceFactory(), client -> client);
		return WebClient.builder().clientConnector(connector).build();
	}

threads webflux-http-kqueue-n and elastic-evictor-1 and elastic-2 are not destroyed.

org.apache.catalina.loader.WebappClassLoaderBase.clearReferencesThreads The web application [ROOT] appears to have started a thread named [webflux-http-kqueue-1] but has failed to stop it. This is very likely to create a memory leak. 
org.apache.catalina.loader.WebappClassLoaderBase.clearReferencesThreads The web application [ROOT] appears to have started a thread named [webflux-http-kqueue-2] but has failed to stop it. This is very likely to create a memory leak.
org.apache.catalina.loader.WebappClassLoaderBase.clearReferencesThreads The web application [ROOT] appears to have started a thread named [elastic-evictor-1] but has failed to stop it. This is very likely to create a memory leak.
org.apache.catalina.loader.WebappClassLoaderBase.clearReferencesThreads The web application [ROOT] appears to have started a thread named [elastic-2] but has failed to stop it. This is very likely to create a memory leak.

[rstoyanchev]

@quaff, please do not report new issues on old tickets. If you can reproduce this with an isolated (small) sample, please create a new issue. If you can't reproduce it easily, then please investigate a little further before opening an issue.

[quaff]

@rstoyanchev I have created #23594