3

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.

jlasarte
  • 623
  • 8
  • 28
  • 1
    Any chance `getLatestTimestamp()` could return null? – Andreas Jan 29 '19 at 21:15
  • @Andreas unfortunately not, the actual value of the timestamp might be null, but the object `Timestamp` is always created. – jlasarte Jan 29 '19 at 21:19
  • That doesn't make any sense. The "value" of a [`Timestamp`](https://docs.oracle.com/javase/8/docs/api/java/sql/Timestamp.html) is a `long`, i.e. a primitive, and cannot be null, so it is impossible to have a non-null `Timestamp` object with a null "value". – Andreas Jan 29 '19 at 21:22
  • Timestamp does not refer to the sql.Timestamp class but to an internal class. I will add this to the main question for clarity. – jlasarte Jan 29 '19 at 21:25
  • Ok, so it contains 2 `long` values. Still doesn't make sense to say its "value" is null, since `long` values **cannot** be null. – Andreas Jan 29 '19 at 21:31
  • You shouldn't be recreating your thread local. Keep one instance and just call `set()` as necessary. – shmosel Jan 29 '19 at 21:32
  • @Andreas i see what you're saying, I misunderstood your comment. Like I said above, the code is heavily edited for the sake of S.O - (what is actually set is a transaction context that has the ts among other things, it was just adding a lot of noice). I might have made it more confusing instead, I apologize. What I can say with certainty is that no - that value cannot be null. – jlasarte Jan 29 '19 at 21:33
  • @shmosel - I am only creating it when it's null, so it doesn't get "recreated". It is only ever one instance and it's calling set(). Unless I'm misunderstanding your point? – jlasarte Jan 29 '19 at 21:36
  • Your check-and-set logic is not atomic. Two threads can initialize it at the same time. That's your race condition. – shmosel Jan 29 '19 at 21:38
  • @shmosel I see - that makes sense. Just for my own edification - looking at the code I would think that even if two initialize it at the same time - when the code proceeds to set the value, the value is set, regardless of the fact that one thread might have overwritten the initialization of the other one. I'm guessing this isn't true and this leaves the ThreadLocal in a bad state? – jlasarte Jan 29 '19 at 21:46
  • The problem is that one of the threads can replace the reference after the other calls `set()`, which results in a missing value. – shmosel Jan 29 '19 at 21:59
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/187527/discussion-between-jlasarte-and-shmosel). – jlasarte Jan 29 '19 at 22:08

1 Answers1

3

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.

jlasarte
  • 623
  • 8
  • 28