MySQL Bugs: #65909: referenceThread causes memory leak in Tomcat

Bug #65909	referenceThread causes memory leak in Tomcat
Submitted:	16 Jul 2012 11:04	Modified:	22 Feb 2013 0:12
Reporter:	Florian Bantner	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / J	Severity:	S3 (Non-critical)
Version:	5.1.21	OS:	Any
Assigned to:	Alexander Soklakov	CPU Architecture:	Any

Description:
The NonRegisteringDriver class starts a thread named 'Abandoned connection cleanup thread' bound to a static context.

This thread prevents Tomcat on WebApp reload from unloading the class properly.

I see this using Hibernate 4.1.5.SP1 and C3P0 Threadpool and Tomcat 7.0.23.

How to repeat:
Write a Servlet using above setting and stop tomcat. It should report: "SEVERE: The web application [] appears to have started a thread named [Abandoned connection cleanup thread] but has failed to stop it. This is very likely to create a memory leak."

Suggested fix:
Don't know enough about Connector/J internals but would suggest some means of shutdown functionallity.

Florian, can you please check if this is a duplicate of Bug#62170?

@Tonci: I would but I don't know how to do so. The jdbc driver internals are a black box to me.

Nevertheless, reading the other bug report my guess is that it's not the same. Judging by the error message it is already the NonRegisteringDriver somehow that's getting loaded. Looking at the sources I see a thread there getting startet but no way of stopping it again.

Digging a little deeper my guess is, that this has to do with me deregsitering c3p0's open drivers directly on application shutdown to get rid of the error messages which are reported in #62170.

I do so by DriverManager.deregisterDriver( drv ); //The found mysql driver

Perhaps this causes the other (I didn't know there was another one until now) driver to get loaded?

On the other hand then this ticket should perhaps be renamed to: "NonRegistering
 starts thread and has no means to stop it"?

Forget my last speculations. I keep getting the message even if I don't remove the mysql driver manually.

This requires a custom shutdown hook to be added to tomcat. Unfortunately, the servlet spec never really addressed context reloading in a portable manner, other than @WebListeners which only work in the newest containers. We should look into delivering a ServletContextListener which would shutdown such threads, however it would require either using a newer servlet container (Tomcat 7 *and* a newer web.xml DTD), or manually changing web.xml. Unfortunately there's no other clean way to do it.

@Marc: Wouldn't it be enough just to provide some kind of "shutdownAll()" function or so and leave the rest to the user? In my case I have the mysqldriver in the application itself and I would have no problem calling MySQL.sth.shutdownAll() in my Servlets destroy() method. I know this is not perfect but still better than not to have any way of getting rid of running threads.

Wouldn't be possible to add a static method to stop the Thread?

The thread could be stopped, for instance, in the ServletContextListener#contextDestroyed (ServletContextEvent) method (in the case of a webapp).

In the source of NonRegisteringDriver one should just replace (in the Thread#run()) the while (true) {...} with a while (volatileVariable) {...} that could be set at "false" by a static method.

Version 5.1.20 didnt have this problem... So I think it could be fixed

Patch for "Abandoned connection cleanup thread"

Attachment: bug65909.diff (text/x-patch), 435 bytes.

Hello Florian,

This looks like the one of the leaks described at http://wiki.apache.org/tomcat/MemoryLeakProtection

Please, try the c/J patch if possible, while I'm trying to reproduce the error.

Hi Alexander,

I think this is what you mean: "Now, if we uncomment the line leakingThread.setContextClassLoader(null); in the above example, tomcat (6.0.24) no longer detect the leak when the application is stopped because the spawned thread context classloader is no longer the webapp's. (the "Find leaks" feature in the manager will report it though)"

If I read this correctly, the patch would only prevent Tomcat from detecting the Leak but won't prevent the Leak from happening.

In order to try this patch I would need a binary because I don't know how to build this thing. (I think I would need some java5/java6 compiler? Im using java7)

Hi,
I’ve tested the patch proposed by Alexander and I can confirm that it only prevents Tomcat from detecting the leak, but the leak is still there. If you do  a thread dump on the tomcat process after undeploying you webapp you will see that the thread “Abandoned connection cleanup thread” is still running.

--
Fabio

After some redeployments one gets a connection failure - it is not only that the thread leaks it is that it apparently does not indeed clean the abandoned connections. I think the severity should be bumped up.

See :
http://mail-archives.apache.org/mod_mbox/tomcat-users/201210.mbox/%3CCABRG_PG%3DS3QJWP5-jY... 

and replies thereof - especially : 

http://mail-archives.apache.org/mod_mbox/tomcat-users/201210.mbox/%3CCABRG_PFrKHhiZcBZ-tSE...

Look out for the line  SEVERE: The web application [/ted2012] appears to have started a thread named [Abandoned connection cleanup thread] but has failed to stop it. This is very likely to create a memory leak.

See how after 10 redeployments (NOT using the app in the meanwhile) one gets the exception :

WARNING: Unexpected exception resolving reference
com.mysql.jdbc.exceptions.jdbc4.MySQLNonTransientConnectionException: Data
source rejected establishment of connection,  message from server: "Too
many connections"

So probably tomcat kills the thread and the connections stay dangling

It's on mysql-connector-java-5.1.22-bin.jar as stated in the beginning of the thread

This situation must be fixed ASAP

Thanks

For those who are experiencing this issue: where is the Connector/J library JAR file?

If the JAR file is in Tomcat's lib/ directory, then there should be *no thread leak* if Alexander's patch is made: the leak does not occur because the webapp's ClassLoader is not stuck in memory. Yes, the thread continues to run but that isn't actually a leak.

If the JAR file is in the webapp's WEB-INF/lib directory, then the webapp's ClassLoader will be the default for the classes loaded therein and the leak will likely occur.

If you use either of Tomcat's connection pools, you should be required to put Connector/J into Tomcat's lib/ directory and therefore Alexander's patch is appropriate.

Palmer, I believe the "too many connections" issue is completely separate from this one.

Hi Christopher,
I my humble opinion even with the patch and even when placing the jar in the $TOMCAT_HOME/lib we still have a problem.
If I un-deploy a web-application that use connector/J I would like the every threads started from my webapp get stopped too: my webapp is not running so there's no reason to keep the "Abbandoned Thred" running on my server.
I think that the correct solution is the one proposed by Marian: the  while (true) {...} loop should be substituted with a while (volatileVariable) {...}. In this way the web-applications could implements a ServletContextListener that simply change the volatile variable to false when the context is destroyed.

Regards

I had my mysql-connector-java-5.1.22-bin.jar in both the WEB-INF/lib and in tomcat's lib folders. Deleted it from my app's web lib - all works as before including the :
SEVERE: The web application [/ted2012] appears to have started a thread named [Abandoned connection cleanup thread] but has failed to stop it. This is very likely to create a memory leak.
and the : "Too many connections"

They may well be not related - but if this thread is killed it may well leave its job unfinished. I really don't know

Hi,

You can try this patch with bundled WebListener. If you use Servlet-3.0 it will be binded automatically. For later spec you should add description to web.xml:

<listener>
   <listener-class>com.mysql.jdbc.StaticThreadsListener</listener-class>
</listener>

You will need a servlet-api.jar to build connector. I used javax.servlet.jar from http://download.oracle.com/otndocs/jcp/servlet-3.0-fr-eval-oth-JSpec/

I tested this with Tomcat 7.0.14, thread lifecycle was checked with profiler. "Abandoned connection cleanup thread" now stops. But despite of normal thread stop, Tomcat still may report a leak because of thread stop is asynchronous to context destruction.

The patch is still under discussion, and I'd like to know does if fit for you.

Thanks.

Hmm... *For later spec* must be *For older spec* of course. Sorry. :)

Hi Alexander,
I've done a minimal tested of you patch under tomcat 7.0.32 and it works as you suggested!
The thread is now stopping but tomcat still complains about the "Abbandoned connection cleanup thread".
I've tried to add referenceThread.setContextClassLoader(null); in the static block of the NonRegisteringDriver but the tomcat leak detector still complains aboout the leak. The only differece is the (false) leak is not automatically detected and printed on the stdout during undeploy, but the (false) leak is detected only when explicit using the "Leak Detector" from the tomcat manager app.
The thread now it's stopping so I don't know why tomcat still complain...

I suspect that tomcat leak detector does its work before "contextDestroyed" event, so thread is still alive at this moment. That's what I've called "thread stop is asynchronous to context destruction", but I'm still trying to realize what exactly happens with that detector.

In anyway, there is now real leak now, thread stops, isn't it?

Nope, the real situation is that context continue destruction right after AbandonedConnectionCleanupThread.shutdown() call, it doesn't wait for real thread shutdown.

You can easily avoid tomcat leak reporting by adding delay right after AbandonedConnectionCleanupThread.shutdown() call (Thread.sleep(3000) for example) in com.mysql.jdbc.StaticThreadsListener or implement destroy() method in your app like this:

@Override
public void destroy() {
   try {
      AbandonedConnectionCleanupThread.shutdown();
      Thread.sleep(3000);
      super.destroy();
   } catch (InterruptedException ex) {
   }
}

But for me it's an overprice of keeping logs clean.

Hi Alexander,
I've tested you new patch and now I don't get any thread leak but my tomcat still complains if a trigger a "find leak detector" because I think it founds a classloader memory leak:
"The following web applications were stopped (reloaded, undeployed), but their
classes from previous runs are still loaded in memory, thus causing a memory
leak (use a profiler to confirm):"

After undeploying my app I've done an heap dump with jvisual vm and I attach a screenshot of the result of the dump analysis done with eclipse mat.
As you can see there's a ContextClassLoader associated to the class AbandonedConnectionCleanupThread.

If I add the line "referenceThread.setContextClassLoader(null);" in the NonRegisteringDriver static block now Tomcat don't complain at all but I dont' think it's a real solution. If I analize the detail of the dump I see some classes of my custom packages still loaded in memory even if my app is undeployed, so I think the classloader leak it's still there...

Classloader memory leak ??

Attachment: leak.png (image/png, text), 193.80 KiB.

Hmm, thread doesn't destroy static self-reference. It's strange that my tomcat didn't report this.

Please, add threadRef = null after join() line in AbandonedConnectionCleanupThread.shutdown() method.

Hi Alexander,
I've added the line "threadRef = null;" as you suggested but I still see the same behavior: if I trigger a "find leak" tomcat complains about classes still loaded in memory but I will try in the next days using a "smaller" webapp in order to exclude other problems...
Thanx for your support

To be sure to get all updates to the "running" variable - should this be made "volatile"?
And what about the connectionPhantomRefs connections - if the thread is going to be shutdown, should the connections forced to be closed via cleanUp before the Thread is going to die?

The issue is that closing the connections may hang on network i/o, so it's a bit tricky to just close them during a context shutdown. It's more than likely better that if they've been abandoned without closing, to somehow log that state (it's an application bug, honestly), and then let network timeouts later clean them up.

Is there any movement on this?

It would be nice to have a public method for shutting-down the driver and its associated threads. Since the thread can be join'd, perhaps a method for initiating the shutdown of everything and another method for waiting on the thread (also maybe waiting with a timeout that just delegates to Thread.join(long).

Oh, and also clearing the context class loader when appropriate.

This really gets in the way. I use Jetty in production instead of Tomcat but the leak is obviously seen in Eclipse MAT. After several context reloads I get PermGen out of memory exception and have to restart the entire container with a bunch of apps instead of taking down just one of them. I'm using Hibernate (with and without C3P0) and GWT. Tomcat handles leaks better though, I used it for detecting the culprit. Those cleanup threads appear in the tomcat's log and dirty killing them in servlet destroy handler doesn't help at all.

Hi,

Patch is included to upcoming 5.1.23 release, just wait a little.

Added to changelog for 5.1.23: 

The cleanup thread for abandoned connections in the
NonRegisteringDriver class was refactored to have a static shutdown
method. Memory was allocated but never released. If you encountered
this leak problem, implement the context listener in your application
with the AbandonedConnectionCleanupThread.shutdown() call in the
contextDestroyed method. This issue was found in applications running
under the Tomcat application server, but it might have also applied
to other application servers. For example:

@WebListener
public class YourThreadsListener implements ServletContextListener {
   public void contextDestroyed(ServletContextEvent arg0) {
      try {
          AbandonedConnectionCleanupThread.shutdown();
      } catch (InterruptedException e) {
      }
   }
   ...
}

Note that if container does not
support annotations, you add the description to web.xml:

   <listener-class>user.package.YourThreadsListener</listener-class>

Note that this fix means that, once shut down, the AbandonedConnectionCleanupThread will never start-up again under a typical webapp configuration (where the JDBC driver is loaded by the container, and not by the webapp).

I noticed that.

In our typical configuration the Thread has been started by the container, as it resides in /lib. Strange enough Tomcat complains about the Thread to be a memory leak when we undeploy. We can stop the Thread with the shutdown method from the app's context listener it will never be started again.

If we do not call the shutdown method of the thread, I don't see how it's creating a memory leak. The thread keeps running and after a redeploy there is still only one instance of the Thread running.

I guess this is a Tomcat issue. Is there an existing bug issue for Tomcat about false memory leak messages that anybody knows of?

> In our typical configuration the Thread has been started by the container, as it resides in /lib. Strange enough Tomcat complains about the Thread to be a memory leak when we undeploy. We can stop the Thread with the shutdown method from the app's context listener it will never be started again.

The problem is that the Thread's context ClassLoader is the WebappClassLoader, which is why Tomcat logs the error. If you don't stop the thread, the WebappClassLoader ends up being pinned in memory after a webapp re-load.

One could argue that Tomcat should not use the WebappClassLoader as the context ClassLoader during DataSource construction, but it's nice to be able to support drivers actually loaded from the webapp's WEB-INF/lib directory, so it's not really fair to declare that Tomcat is wrong, here.

I've suggested an alternative technique that Tomcat could use here:
http://markmail.org/message/dmvlkps7lbgpngil

> If we do not call the shutdown method of the thread, I don't see how it's creating a memory leak. The thread keeps running and after a redeploy there is still only one instance of the Thread running.

Correct, but that Thread has a reference to the old WebappClassLoader, which would otherwise be eligible for garbage-collection.

> I guess this is a Tomcat issue. Is there an existing bug issue for Tomcat about false memory leak messages that anybody knows of?

No, it's not a false memory leak: it's a real one.

Thanks for you extensive reply Chris. I see your point and it totally makes sense. I figured that the loading of the driver should initiated by the application that has other similar responsibilities. In our setup that is the master application node, and not one of the worker instances. So we will implement driver loading and maintenance in our master node.

I think there's still a problem here. It's a well known good practice to avoid memory leaks to do the following when JDBC drivers and bundled with your webapp (in WEB-INF/lib):
- register drivers on context startup (if automatic registration does not occur)
- unregister drivers on context shutdown
This should guarantee that any required cleanup process for the registered drivers is performed.
Unfortunately, as of Connecto/J 5.1.34, the shutdown method of the AbandonedConnectionCleanupThread is not called when the MySql driver is unregistered. One does has to know that there's this problem to write yet another listener (specific to MySql, because it has a compile dependency to MySql Connector/J) to call that shutdown method. 

IMHO, the driver cleanup process should be just an implementation detail once the caller performs the necessary shutdown operations provided by the JDBC API.