How to clean up Java ThreadLocals in accordance with Sonar?

Question

A Sonar rule available since Aug 21, 2019 (squid:S5164 / RSPEC-5164) mandates to clean up "ThreadLocal" variables when no longer used. So, let's take the following class (JDK6 compatible):

public class ThreadLocalExample {

    private static final ThreadLocal<NumberFormat> formats = new ThreadLocal<NumberFormat>() {
        @Override
        protected NumberFormat initialValue() {
            final NumberFormat nf = NumberFormat.getNumberInstance(Locale.US);
            nf.setMinimumFractionDigits(2);
            nf.setMaximumFractionDigits(2);
            nf.setGroupingUsed(false);
            return nf;
        }
    };

    public static NumberFormat getFormatter() {
        return formats.get();
    }
}

Sonar reports a major bug on the ThreadLocal declaration, with the following explanation:

"ThreadLocal" variables should be cleaned up when no longer used

ThreadLocal variables are supposed to be garbage collected once the holding thread is no longer alive. Memory leaks can occur when holding threads are re-used which is the case on application servers using pool of threads.

To avoid such problems, it is recommended to always clean up ThreadLocal variables using the remove() method to remove the current thread’s value for the ThreadLocal variable.

Now, I adopted the ThreadLocal approach in order to reuse NumberFormat instances as much as possible, avoiding the creation of one instance per call, so I think if I called remove() somewhere in the code, I would lose all the advantages of this solution. Am I missing something? Thanks a lot.

Have you actually profiled whether there is a difference between using a `ThreadLocal`, and simply creating a new instance and returning it each time `getFormatter` is invoked? It seems like it would be an awful lot easier to reason about the life cycle of the instances if you were to do that. — Andy Turner, Sep 01 '19 at 22:17

score 7 · Accepted Answer · edited Mar 18 '23 at 03:23

Sonar is right here.

Each thread will have its own ThreadLocal state and so its own instance of NumberFormat.
So in the general case it may be undesirable to not clear data from the state since the thread may be reused (recycled by the server) and the state valued for the previous client may be inconsistent for the current client.
For example some clients could have the format US, others the format FR, and so for... Besides some threads could instantiate that ThreadLocal class, other no. But by not cleaning the state, the state will still use memory for threads that may not need them.

Well, in your code, there is not variability of the ThreadLocal state since you set the state for any instance, so no inconsistency risk is likely, just memory "waste".

Now, I adopted the ThreadLocal approach in order to reuse NumberFormat instances as much as possible, avoiding the creation of one instance per call

You reuse the ThreadLocal state by a thread request basis.
So if you have 50 threads, you have 50 states.
In web applications, the server maps the client HTTP request to one thread.
So you don't create multiple instances of the formatter only in the scope of 1 http request. It means that If you use the formatter one or two time by request processing, the ThreadLocal cache doesn't bring a great value. But if you use it more, using it makes sense.

so I think if I called remove() somewhere in the code, I would lose all the advantages of this solution

Calling remove() will not hurt performance if you do that when the request processing is done. You don't lose any advantage since you may use the formatter dozen of times in the scope of the request and it will be cleaned only at the end.

You have Request Listener in the servlet specification : https://docs.oracle.com/javaee/7/api/javax/servlet/ServletRequestListener.html.
You could do that in void requestDestroyed(ServletRequestEvent sre).

score 4 · Answer 2 · answered Sep 01 '19 at 20:36

You should not call #remove directly after you used the formatter. As you wrote that would defeat the purpose.

You only need to call #remove in the following case. Your web application is unloaded from the application server e.g. Tomcat. But the application server itself keeps on running.

In this case the application server probably keeps the threads it created for your application around and these threads will still have each an instance of NumberFormat associated with them. That's your memory leak.

So if you always restart the entire app server you probably don't need to care about this problem.

If you want to clean up the ThreadLocal properly you would want to call #remove once your application is starting to shut down. This way you reused the NumberFormat instance a maximum of times while still cleaning up properly.

score 1 · Answer 3 · answered Sep 01 '19 at 20:42

The link you provided says the following:

Memory leaks can occur when holding threads are re-used which is the case on application servers using pool of threads.

For example you build a pool with 7 threads because your CPU has 8 cores. Then you submit a task to the pool and it would be solved by one of the threads. Afterwards the thread may do not have other tasks to do, but the thread still holds the ThreadLocal object. This would be a waste of memory and because ThreadLocal has a reference to the containing object the garbage collector can not remove the containing object (could result in a memory leak).

If you do not reuse the thread and cleanup all references to the thread , then there will not be a memory leak. If you reuse the thread later the threadLocal object will be in-memory until it is overwritten or cleaned up or until the thread is destroyed.

How to clean up Java ThreadLocals in accordance with Sonar?

3 Answers3