ActivityCorrelator is causing a classloader leak

[JohnA2]

ActivityCorrelator uses ThreadLocal, but it never calls ThreadLocal.remove() to clean it up. As a result, if a thread pool is used, the classloader that loaded ActivityCorrelator can never be garbage collected thus creating a memory leak.

Example of the path from GC root:

this     - value: org.apache.catalina.loader.WebappClassLoader #3
 <- <classLoader>     - class: com.microsoft.sqlserver.jdbc.ActivityId, value: org.apache.catalina.loader.WebappClassLoader #3
  <- <class>     - class: com.microsoft.sqlserver.jdbc.ActivityId, value: com.microsoft.sqlserver.jdbc.ActivityId class ActivityId
   <- value     - class: java.lang.ThreadLocal$ThreadLocalMap$Entry, value: com.microsoft.sqlserver.jdbc.ActivityId #2
    <- [54]     - class: java.lang.ThreadLocal$ThreadLocalMap$Entry[], value: java.lang.ThreadLocal$ThreadLocalMap$Entry #137
     <- table     - class: java.lang.ThreadLocal$ThreadLocalMap, value: java.lang.ThreadLocal$ThreadLocalMap$Entry[] #28
      <- threadLocals (thread object)     - class: org.apache.tomcat.util.threads.TaskThread, value: java.lang.ThreadLocal$ThreadLocalMap #28

[v-nisidh]

@JohnA2 : Can you check if #321 PR prevents class loader leak?

If it's not fixed then let me know.

[v-nisidh]

@JohnA2 : I able to detect leakage in with my patch so I am closing PR right now. After my fixes I will open same or new PR.

Thanks @JohnA2 for opening this issue.

[JohnA2]

Thanks for the quick fix, @v-nisidh! The leak in my particular case is fixed, because I don't use a connection pool for the task I'm using mssql-jdbc for. I use Spring Framework's JdbcTemplate to open a new connection, execute a single SQL query and close the connection. All this happens in the same thread, so the current fix works just fine.

However, as @marschall mentioned in #321, this fix won't work for a more traditional case with a web application running on an application server that uses a connection pool, because different threads can be used to open and close the same connection, and ThreadLocal.remove() doesn't have an effect as it can only clean up ThreadLocal for the thread it's called from.

Here's the source code for ThreadLocal.remove()

public void remove() {
	ThreadLocalMap m = getMap(Thread.currentThread());
	if (m != null)
		m.remove(this);
}

As you can see it uses the ThreadLocalMap from the current thread, so there's no easy way to clean up another thread's ThreadLocal. I would suggest considering using InheritableThreadLocal instead of ThreadLocal to avoid this limitation or even better not use ThreadLocals at all, if possible.

@marschall You are correct that it's recommended to deploy JDBC drivers to the application server itself, but it's only because of the way DriverManager works, see https://stackoverflow.com/questions/6981564/why-must-the-jdbc-driver-be-put-in-tomcat-home-lib-folder. But it's no longer a problem if you unregister the driver from DriverManager during ServletContext shutdown. There are some cases when it's useful because it's undesirable or impossible to modify the application server's configuration. However leaky JDBC drivers create a problem in this scenario. The leak still exists when the driver is deployed on the application server level, it's just hidden because the driver classes are loaded by the app server's own classloader, which is never supposed to be garbage collected anyway. Other JDBC drivers I used - Oracle and MySQL don't have a leak.

[peterbae]

Thanks for the contribution. as @JohnA2 suggested, I believe it would be the best approach to remove using ThreadLocal in this case, for a couple of reasons:

1. As discussed in the comments, if the user has a web application running on an application server with a connection pool, it is difficult to signal the individual worker threads to call ThreadLocal.remove() since the thread that manages the connection pool is likely not a worker thread, and calling close() on the thread that manages the connection pool will affect the worker thread.

2. We don't necessarily need ThreadLocal in this case since the ThreadLocal was being used for conveniently associating a thread with its own ActivityId object, and was not being used to increase performance and we can achieve thread safety in a different approach. Therefore, having a static hashmap of <id, ActivityId> achieves the same goal without introducing classloader leak by using ThreadLocal.

I opened a new PR for this issue: #465