Questions about using ThreadLocal in a Spring singleton scoped service

Question

In my singleton scoped service class below, all methods in the class require some user context that is known when Service.doA() is called. Instead of passing around info across methods, I was thinking about storing those values in TheadLocal. I have two questions about this approach:

1) Does the implementation below use ThreadLocal correctly? That is, it is thread-safe and the correct values will be read/written into the ThreadLocal?

2) Does ThreadLocal userInfo need to be cleaned up explicitly to prevent any memory leaks? Will it be garbage collected?

@Service
public class Service {
    private static final ThreadLocal<UserInfo> userInfo = new ThreadLocal<>(); 

    public void doA() {
        // finds user info
        userInfo.set(new UserInfo(userId, name));
        doB();
        doC();
    }

    private void doB() {
        // needs user info
        UserInfo userInfo = userInfo.get();
    }

    private void doC() {
        // needs user info
        UserInfo userInfo = userInfo.get();
    }
}

If you're using singleton beans, just make the field an instance field. — Sotirios Delimanolis, May 19 '16 at 23:51
@SotiriosDelimanolis But having a UserInfo instance field is not thread-safe, no? — Glide, May 20 '16 at 17:40
Whether it's an instance field or a class field changes nothing regarding its thread safety. Also, it's not a `UserInfo` field, it's a `ThreadLocal` field. `ThreadLocal` is thread safe. And the field would be `final`. — Sotirios Delimanolis, May 20 '16 at 17:44
@SotiriosDelimanolis Sure I guess I was confused by your first comment because `userInfo` is already an instance field. — Glide, May 20 '16 at 20:00
I'm referring to `private static final ThreadLocal userInfo = new ThreadLocal<>();`. That is a class field. Get rid of the `static`. You're only ever going to access it through the `Service` bean which is a singleton, so there's no reason to leave it `static`. — Sotirios Delimanolis, May 20 '16 at 20:01

Nathan Hughes · Accepted Answer · 2016-05-21T15:56:48.083

1) The example code is ok, except for the name clashes in doB and doC where you're using the same name for the static variable referencing the ThreadLocal as you are for the local variable holding what you pull from the ThreadLocal.

2) The object you store in the ThreadLocal stays attached to that thread until explicitly removed. If your service executes in a servlet container, for instance, when a request completes its thread returns to the pool. If you haven't cleaned up the thread's ThreadLocal variable contents then that data will stick around to accompany whatever request the thread gets allocated for next. Each thread is a GC root, threadlocal variables attached to the thread won't get garbage-collected until after the thread dies. According to the API doc:

Each thread holds an implicit reference to its copy of a thread-local variable as long as the thread is alive and the ThreadLocal instance is accessible; after a thread goes away, all of its copies of thread-local instances are subject to garbage collection (unless other references to these copies exist).

If your context information is limited to the scope of one service, you're better off passing the information around through parameters rather than using ThreadLocal. ThreadLocal is for cases where information needs to be available across different services or in different layers, it seems like you're only overcomplicating your code if it will be used by only one service. Now if you have data that would be used by AOP advice on different disparate objects, putting that data in a threadlocal could be a valid usage.

To perform the clean-up typically you would identify a point where the thread is done with the current processing, for instance in a servlet filter, where the threadlocal variable can be removed before the thread is returned to the threadpool. You wouldn't use a try-finally block because the place where you insert the threadlocal object is nowhere near where you are cleaning it up.

Nicolas Filotto · Answer 2 · 2016-05-20T08:02:48.463

When you use a ThreadLocal you need to make sure that you clean it up whatever happens because:

It creates somehow a memory leak as the value cannot be collected by the GC because an object is eligible for the GC if and only if there is no object anymore that has an hard reference directly or indirectly to the object. So for example here, your ThreadLocal instance has indirectly an hard reference to your value though its internal ThreadLocalMap, the only way to get rid of this hard reference is to call ThreadLocalMap#remove() as it will remove the value from the ThreadLocalMap. The other potential way to make your value eligible for the GC would be the case where your ThreadLocal instance is itself eligible for the GC but here it is a constant in the class Service so it will never be eligible for the GC which is what we want in your case. So the only expected way is to call ThreadLocalMap#remove().
It creates bugs hard to find because most of the time the thread that uses your ThreadLocal is part of a thread pool such that the thread will be reused for another request so if your ThreadLocal has not properly been cleaned up, the thread will reuse the instance of your object stored into your ThreadLocal that is potentially not even related to the new request which leads to complex bugs. So here for example we could get the result of a different user just because the ThreadLocal has not been cleaned up.

So the pattern is the following:

try {
    userInfo.set(new UserInfo(userId, name));
    // Some code here 
} finally {
    // Clean up your thread local whatever happens
    userInfo.remove();
}

About thread safety, it is of course thread safe even if UserInfo is not thread safe because each thread will use its own instance of UserInfo so no instance of UserInfo stored into a ThreadLocal will be accessed or modified by several threads because a ThreadLocal value is somehow scoped the the current thread.

Thanks for your thorough response. 2 follow up questions. 1) How come the object stored by `ThreadLocal` cannot be cleaned up by GC? 2) Are you saying that in my example if `TheadLocal.remove()` was never called, the code becomes not thread-safe? (i.e. in `doB()` I could be getting another userInfo) — Glide, May 19 '16 at 22:56
I improved my answer for #1, for #2 I only say that If you don't call TheadLocal.remove(), you will face the issues mentioned above, the class TheadLocal is thread safe and will remain thread-safe even if you don't call remove(). — Nicolas Filotto, May 20 '16 at 08:05

score 1 · Answer 3 · answered May 19 '16 at 19:20

It definitely should be cleaned up after use. ThreadLocals leak memory extremely easily, both heap memory and permgen/metaspace memory via classloders leaking. In your case the best way would be:

public void doA() {
  // finds user info
  userInfo.set(new UserInfo(userId, name));
  try {
    doB();
    doC();
  } finally {
    userInfo.remove()
  }
}

Questions about using ThreadLocal in a Spring singleton scoped service

3 Answers3