javamultithreadingoptimistic-concurrency

ThreadLocal.get() returning null, even when I'm initializing it before


I feel like I might be misunderstanding how this should work.

I have this code:

public Timestamp startTransaction() {
    cleanupTransactionContext();
    Timestamp timestamp = getLatestTimestamp();
    initThreadLocalVariables(timestamp);
    return getTransactionContext().toString();
}

private Timestamp getTransactionContext() {
    if (transactionContext == null) {
        throw new BadTransactionStateException();
    }
    return transactionContext.get();
}

private void initThreadLocalVariables(Timestamp timestamp) {
    setTransactionContext(timestamp);
}

private void setTransactionContext(Timestamp ts) {
    if (this.transactionContext == null) {
        this.transactionContext = new ThreadLocal<>();
    }
    this.transactionContext.set(ts);
}

It is my understanding that ThreadLocal.get() should never return null (from JDK):

/**
 * Returns the value in the current thread's copy of this
 * thread-local variable.  If the variable has no value for the
 * current thread, it is first initialized to the value returned
 * by an invocation of the {@link #initialValue} method.
 *
 * @return the current thread's value of this thread-local
 */
public T get() {
    Thread t = Thread.currentThread();
    ThreadLocalMap map = getMap(t);
    if (map != null) {
        ThreadLocalMap.Entry e = map.getEntry(this);
        if (e != null) {
            @SuppressWarnings("unchecked")
            T result = (T)e.value;
            return result;
        }
    }
    return setInitialValue();
}

Because I've explicitly set it before in setTransactionContext, which in turns call ThreadLocal.set, that should be creating the map:

   /**
 * Sets the current thread's copy of this thread-local variable
 * to the specified value.  Most subclasses will have no need to
 * override this method, relying solely on the {@link #initialValue}
 * method to set the values of thread-locals.
 *
 * @param value the value to be stored in the current thread's copy of
 *        this thread-local.
 */
public void set(T value) {
    Thread t = Thread.currentThread();
    ThreadLocalMap map = getMap(t);
    if (map != null)
        map.set(this, value);
    else
        createMap(t, value);
}

However, sometimes I am getting null pointer exceptions in: return getTransactionContext().toString();. Other times it works perfectly, so I suspect some kind of race condition, I just fail to see what it could be.

PS: The Timestamp class looks like this:

public final class Timestamp {

private final long timeInMilliseconds;
private final long sequenceNumber;

}

But please note this is a simplified version of the code that doesn't include several checks to make sure this isn't null. getLatestTimeStamp value itself is correct and won't be set to null.


Solution

  • As pointed out by @shmosel - the problem was this piece of code wasn't atomic:

    private void setTransactionContext(Timestamp ts) {
      if (this.transactionContext == null) {
        this.transactionContext = new ThreadLocal<>();
      }
      this.transactionContext.set(ts);
    }
    

    So two threads might be creating ThreadLocal and interfering with each other. Moving the creation of the thread local to the variable declaration solves the issue, as subsequent operations on ThreadLocal are thread safe by default.