Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebClient does not clean up underlying resources #23594

Closed
quaff opened this issue Sep 6, 2019 · 8 comments
Closed

WebClient does not clean up underlying resources #23594

quaff opened this issue Sep 6, 2019 · 8 comments
Assignees
Labels
for: external-project Needs a fix in external project

Comments

@quaff
Copy link
Contributor

quaff commented Sep 6, 2019

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.

Please run tests in attachment WebClientTest.zip.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 6, 2019
@rstoyanchev rstoyanchev changed the title WebClient is not proper clean up underlying resources WebClient does not clean up underlying resources Sep 6, 2019
@rstoyanchev
Copy link
Contributor

Thanks for the sample. I verified this is not a regression and that the behavior is from the start in 5.1.0. It does clean up but it is triggered asynchronously. I added a 5 second delay and the threads were no longer there. I suppose we could change this to disposeLater().block() but the important point is that the cleanup does occur.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Sep 6, 2019
@quaff
Copy link
Contributor Author

quaff commented Sep 9, 2019

webflux-http-kqueue-* is destroyed after some delay, but elastic-evictor-1 and elastic-2 are never destroyed.
How to reduce the delay?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 9, 2019
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Sep 9, 2019

How to reduce the delay?

I don't think you can speed it up. It's how long it takes to shut down. At least in terms of Reactor Netty APIs I don't see anything else we can do from the Spring Framework side.

About the elastic-evictor-1 and elastic-evictor-2, those seem to be created by Reactor Core whenever the ElasticScheduler is used, which could be via Schedulers.elastic() for example. Are you using that anywhere in your code perhaps?

@smaldini any idea about the the evictor threads? I see some use of Schedulers.elastic() from PooledConnectionProvider in disposeLater(). Shouldn't such a method, intended for cleaning resources, avoid the use of a global scheduler that creates more threads?

@quaff
Copy link
Contributor Author

quaff commented Sep 10, 2019

@rstoyanchev I'm not using Schedulers.elastic(), It started by WebClient, that's all my code in test case.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Sep 13, 2019

I've created reactor/reactor-netty#846 for the elastic evictor threads but I'll leave this open since depending on the fix, we may have a follow-up change.

In addition we'll also make an improvement with #23631 to ensure that we wait until threads are stopped.

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 13, 2019
@rstoyanchev rstoyanchev added this to the 5.1.10 milestone Sep 13, 2019
@rstoyanchev rstoyanchev self-assigned this Sep 16, 2019
@bclozel
Copy link
Member

bclozel commented Sep 17, 2019

Should this be targeted against our 5.2.X milestone?
The fix in reactor has been done in their 0.9.x generation (Dysprosium) and our own 5.1.x branch depends on Californium (reactor netty 0.8.x).

@quaff
Copy link
Contributor Author

quaff commented Sep 18, 2019

after commit ca2b2f5 , webflux-http-kqueue-* is destroyed after context closed, but there is a thread named globalEventExecutor-1-1 not destroyed immediately, should wait for 1 second.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Sep 20, 2019

after commit ca2b2f5 ...

@quaff please comment under #23631, which is the issue for that commit.

Should this be targeted against our 5.2.X milestone?

Either way it's a change for Reactor Netty to make to ensure the global elastic scheduler resources aren't used. I only kept this issue open in case their change required us to use a different API. However the change made is transparent for us so this issue can be closed. If it needs to work for 5.1.x (and Reactor Netty 0.8.x) then that change can only come from Reactor Netty.

@rstoyanchev rstoyanchev removed this from the 5.1.10 milestone Sep 20, 2019
@rstoyanchev rstoyanchev added for: external-project Needs a fix in external project and removed in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project Needs a fix in external project
Projects
None yet
Development

No branches or pull requests

4 participants