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?

Incardination

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. – Triparted

Answer 1

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.)

Earlearla

Exactly. The only source of race condition I can think of is the method 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) – Triparted

@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. – Earlearla

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. – Triparted

@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. – Incardination

@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. – Incardination

@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? – Earlearla

@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. – Incardination

@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. – Earlearla

@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. – Incardination