Hi,
I'm attaching a a modified version of the graphql-server project from the Spring Getting Started Guide. The only real modifications are the addition of Spring Data & H2 as dependencies, plus some standard configuration. When deploying the application as a WAR file to 10x Version Tomcat it would appear the memory is not being released following shutdown of the application.
I'm only able to reproduce the problem when using spring data and spring graphql in tandem. I cannot reproduce the problem when using one without the other.
Comment From: bclozel
I think that calling Schedulers.boundedElastic() will create a shared instance that will be kept around until the JVM is shut down. Have you tried to shut them down with Schedulers.shutdownNow() as part of the application lifecycle when the application is undeployed? Typically, you could create a bean that implements SmartLifecycle and calls this shutdown method in its stop() implementation. You can of course use any other mechanism supported by you environment (like the Servlet lifecycle).
Can you try this and let us know if this works out?
Comment From: michaelpaul27
That works, thanks. :)
Is this bleeding out from the framework internals? I don't see the demo app explicitly initiating anything via Scheduler and didn't realize a shutdown hook was needed.
Comment From: bclozel
Is this bleeding out from the framework internals? I don't see the demo app explicitly initiating anything via Scheduler and didn't realize a shutdown hook was needed.
So, to summarize:
* Spring libraries call Schedulers.boundedElastic() for wrapping blocking operations. This is a static call that shares scheduler instances for the entire lifetime of the JVM.
* This application is deployed as a WAR on a Servlet container and can be undeployed/redeployed multiple times without shutting down the JVM. Undeploying the application leaks the classloader of this application, since resources are still tied to static instances
As far as I understand, we can safely shut down schedulers in such an environment, as instances are tied to the application classloader and not the entire JVM. So there is no risk of shutting down a scheduler that's currently used by another application currently running on the same Servlet container.
As a first step here, I'd like to consider the lifecycle angle of this case and ask @chemicL for some thoughts on the case here:
- Spring libraries merely use the
Schedulers.boundedElastic()static call for wrapping blocking operations. Maybe such WAR deployments should be using a custom factory as they expect a different resource management model? - Should WAR applications manually shut down instances as part of the application lifecycle?
- Should we consider documentation improvements in Reactor and/or Spring Boot about this?
- Should Spring Boot consider shutting down instances as part of the application lifecycle in all cases?
I have just discussed this with @rstoyanchev and we will consider enhancements to only call Schedulers.* when it's needed at runtime and prevent such cases to happen in the first place when schedulers are not strictly needed. This should lead to new issues in separate projects.
Comment From: rstoyanchev
There is the broader lifecycle angle question to be considered. Even a Spring for GraphQL application that's using Spring MVC as a transport could need a Scheduler in some cases, e.g. in the synchronous client or the WebSocket handler.
Specifically for the sample here, however, a Scheduler doesn't actually need to be used, and seems to be created eagerly. I've created https://github.com/spring-projects/spring-framework/issues/33218 to resolve that.
Comment From: chemicL
We had a brief exchange with @bclozel about the points raised above. I'm happy to help in Framework's/Boot's lifecycle management of the Scheduler instances. One idea that comes to mind is to not replace the static ones, but explicitly use specific Scheduler implementations in Spring which are in control of the framework. They would not create risks of colliding with 3rd party library expectations or user-defined long running periodic tasks or shutdown hooks. The drawback for Spring would be the necessity to always feed operators with these instances. Also it could be perceived as a breaking change for users. We can discuss this further and consider various options.
Comment From: bclozel
I have discussed this with the Spring Boot team, and we think this case is quite similar to spring-projects/spring-boot#21221. So we would like to consider expanding the SpringBootServletInitializer to also defensively shut down the shared Schedulers once the application context has been closed.
As a first step, I will use the sample you provided to test this approach and then apply the appropriate change. In the meantime, please consider adding a ServletContextListener bean to your application that calls Schedulers.shutdownNow() when the servlet context is destroyed.
Transferring this issue to Spring Boot.