Deregister JDBC drivers during undeploy of a war deployment

[BenICE]

Hi,

following up on the registration of JDBC drivers in Spring Boot (especially #2612).

As I can see it, no Spring Boot Application (e.g. 2.2.6) deregisters its JDBC driver.
Standard Spring Boot created with Spring boot initializr & In Memory H2 database, will leave its driver behind, which leads to a memory leak as the class loader cannot be GCed.

Why is there no huge outcry?

• Running with an embedded tomcat does not show the problem as it is anyway restarted
• Running in a stand-alone tomcat does not show the problem (or only a warning) as the memory leak detection of tomcat will take care of it, just logging a severe log message (https://stackoverflow.com/questions/3320400/to-prevent-a-memory-leak-the-jdbc-driver-has-been-forcibly-unregistered)

I guess these are the most standard cases, but if you run it in a JBoss environment you will not have much fun as the server will quickly rise an OOM after some deployments.

Workaround (which I deployed) is to deregister the driver in a context listener, when the context is destroyed (much like https://stackoverflow.com/questions/3320400/to-prevent-a-memory-leak-the-jdbc-driver-has-been-forcibly-unregistered/23912257#23912257).

Sure, we could relay on the Tomcat memory leak protection or deploy the database driver in the application server, but I would assume a War-file produced by Spring Boot should run everywhere.

What is your take on it?

Best

[wilkinsona]

@BenICE Thanks for opening a follow-up issue. As I requested on #2612, can you please provide a minimal sample that reproduces the problem?

[BenICE]

Sorry, it is really nothing special.
Just a Spring Boot app (created by the initializr) & application.properties to configure the datasource.
Deploy this app in a stand alone tomcat and you will see that tomcat (e.g. latest 9) need to tidy up after us. Do this in a JBoss (e.g. latest 7.3) and you will create a memory leak.

https://github.com/BenICE/exampleMemoryLeak <-- hope this is the right way to share it.

[wilkinsona]

Thanks for the sample, @BenICE. Unfortunately, I don't think there's much that we can do about this.

H2 automatically registers itself in the static initialiser of org.h2.Driver. It does this by calling its own load() method. While public, this method is documented as INTERNAL. There's a corresponding unload() method that deregisters the driver. It too is labelled as INTERNAL and H2 never calls it itself. Furthermore, the javadoc of the org.h2.Driver class says that "an application should not use this class directly". Given this, it's not clear to me how H2 is intended to be used in a way that does not cause a memory leak unless you ignore the documentation and call internal API.

A further complication is that Spring Boot itself does not load org.h2.Driver or create an instance of it. It's Hikari that does this when creating its DriverDataSource. It might be possible for a change to be made to Hikari to deregister the Driver instance that it creates when the connection pool is being closed, however I don't think this could be used safely with H2 is it maintains internal state for its registration and this will get out of sync if its Driver is deregistered from the DriverManager by any mechanism other than calling H2's internal unload() method.

I think you have a few options:

• Switch to a database other than H2
• Use a Tomcat- or JBoss-managed data source retrieved from JNDI
• Add some code to your app to clean up H2 as you've suggested above

FWIW, I would implement that last of these by overriding onStartup in your SpringBootServletInitializer sub-class and registering a ServletContextListener:

@Override
public void onStartup(ServletContext servletContext) throws ServletException {
	servletContext.addListener(new ServletContextListener() {

		@Override
		public void contextDestroyed(ServletContextEvent sce) {
			org.h2.Driver.unload();
		}
			
	});
	super.onStartup(servletContext);
}

I've resorted to using internal API to avoid potential problems with H2's internal state getting out of sync with that of DriverManager.

[BenICE]

Shit sorry...I lead you on a goose chase. I just took H2 as an example.
Yes, they have their internal state, which is messy.

But you have the same behaviour as well with e.g. the maria DB driver. In my opinion it concerns all JDBC drivers.

[wilkinsona]

Ideally, JDBC drivers would ship with a ServletContainerInitializer so that they can automatically unregister themselves when the servlet context is being destroyed. This would avoid any problems with internal state getting out of sync with DriverManager state and fix the problem for everyone in any servlet environment. Interestingly, a number of drivers ship with OSGi BundleActivators to address the same problem in an OSGi environment.

Failing that, fixing the problem at the container level is the next best option, but it limits the impact of the fix to those that are using a particular container.

Lastly, we could fix it in Spring Boot by adding code that mimics what Tomcat does in JdbcLeakPrevention. Something like this would work, I believe, in any servlet container:

@Override
public void contextDestroyed(ServletContextEvent sce) {
	for (Driver driver: Collections.list(DriverManager.getDrivers())) {
		if (driver.getClass().getClassLoader() == sce.getServletContext().getClassLoader()) {
			try {
				DriverManager.deregisterDriver(driver);
			} catch (SQLException ex) {
				// Continue
			}
		}
	}
}

JdbcLeakPrevention contains a comment about DriverManager.getDrivers() causing additional registration. As far as I can tell, that's no longer the case (on Java 8 at least).

The code above would break any Driver that keeps its own internal registration state as it would think that it is registered when it is not. If we can guarantee that we perform deregistration after the application context has closed, or at least after all beans have been destroyed, this probably won't cause a problem as the Driver's class is about to get garbage collected as a result of the web app being undeployed.

I'm wavering a bit on this one now. Let's see what the rest of the team thinks.