Do operations on ThreadLocal have to be synchronized?

Question

Here is the code I've stumbled across:

class TransactionContextHolder {

private static final ThreadLocal<TransactionContext> currentTransactionContext = new NamedInheritableThreadLocal<TransactionContext>(
    "Test Transaction Context");


static TransactionContext getCurrentTransactionContext() {
    return currentTransactionContext.get();
}

static void setCurrentTransactionContext(TransactionContext transactionContext) {
    currentTransactionContext.set(transactionContext);
}

static TransactionContext removeCurrentTransactionContext() {
    synchronized (currentTransactionContext) {
        TransactionContext transactionContext = currentTransactionContext.get();
        currentTransactionContext.remove();
        return transactionContext;
    }
}

}

The currentTransactionContext field is of type ThreadLocal and it is the only field in the class.

It seems to me that synchronization is not needed here because value stored in ThreadLocal is associated with particular thread and thus it's not a shared state. In addition I think it impacts performance as currentTransactionContext itself is shared and only one thread is allowed to enter the block while many can do it in parallel without impacting correctness.

Is the synchronization needed here?

Just be sure that `currentTransactionContext.initialValue` (or even the getter) doesn't have a shared state and then you should be fine with removing the synchronisation. — Margaret Bloom, Dec 25 '16 at 19:11

score 9 · Accepted Answer · edited May 23 '17 at 12:33

9

In general, it's hard to make guarantees about threadsafety given only a tiny snippet of a program, since threadsafety is a property of the whole program, and synchronized can coordinate behavior across many different parts of a program.

For example: maybe there's some other piece of code somewhere else that uses crazy unsafe reflection to try to inspect and/or mutate the guts of the ThreadLocal, and that will therefore break if you mutate the ThreadLocal without locking?

Realistically, though, you are quite right: there's never any reason to synchronize on a ThreadLocal instance, except perhaps inside its initialValue method. ThreadLocal is itself a threadsafety mechanism, and it manages its threadsafety better than you could get by tacking on synchronized anyway.

(Hat-tip to Margaret Bloom for pointing out the initialValue case.)

edited May 23 '17 at 12:33

Community

1
1

answered Dec 25 '16 at 18:49

ruakh

175,680
26
273
307

Exactly. The only source of race condition I can think of is the method [`initialValue`](https://docs.oracle.com/javase/7/docs/api/java/lang/ThreadLocal.html#initialValue()) of `ThreadLocal`. This is shared across all the threads and, as seen in the example in the doc linked, it must use some protection. This method is called by `get` since they always perform a `remove` after it (don't know why) – Margaret Bloom Dec 25 '16 at 18:52
@MargaretBloom: Re: "they always perform a `remove` after it (don't know why)": Well, what the OP posted is the `removeCurrentTransactionContext` method, so I think it makes sense that it always removes the current transaction context. – ruakh Dec 25 '16 at 19:02
Right, I overlooked the method name :) The `remove` makes sense, it is the `get` that's suspicious. If there is no context yet, that method may potentially create a new one and then remove it. But as you said: it's only a tiny snippet. – Margaret Bloom Dec 25 '16 at 19:09
@MargaretBloom: In this particular case initValue is not overrided so it set null but you're right in general I should take into account initialValue. – user1301558 Dec 26 '16 at 13:02
@ruakh: Regarding mutateing state of class elsewhere the synchronized block in one place doesn't protect you. You have to use synchronization in a place which mutates state too. For example if someone invoked `setCurrentTransactionContext` method and it was executed right after `currentTransactionContext.remove()` then it would make remove method return outdated value of context. However in this case it is ThreadLocal, `setCurrentTransactionContext` and `removeCurrentTransactionContext` methods have to be invoked by the same thread in the same time which is impossible. – user1301558 Dec 26 '16 at 13:10
@user1301558: Sorry, I'm not sure which part of my answer you're trying to reply to. Can you highlight the specific bit that you didn't understand? – ruakh Dec 26 '16 at 17:17
@ruakh: I referred to the example you gave and tried to say that in general your code which is synchronized doesn't protect you if somewhere else is a piece code which does something unsafe (mutates state without synchronization). But it's rather more general and it's not the case for ThreadLocal as you said. – user1301558 Dec 26 '16 at 18:50
@user1301558: I didn't say anything about mutating state *without synchronization*; I'm not sure where you got that from. My point was that you had posted only a tiny snippet of code, and that the `synchronized` might be needed because of its interaction with some other part of the code that you hadn't posted. – ruakh Dec 26 '16 at 19:06
@ruakh: Ok, I misunderstood you as I imagined this interaction as something mutating state without being synchrnized itself but it doesn't need to be true. – user1301558 Dec 26 '16 at 19:32

Do operations on ThreadLocal have to be synchronized?

1 Answers1

Linked