CODEWITHSUNDEEP
javamultithreadingthread-safety
Maria
Maria

Reputation: 604

Not thread safe class

Why below class is not thread safe ?

public class UnsafeCachingFactorizer implements Servlet {
    private final AtomicReference<BigInteger> lastNumber = new AtomicReference<>();
    private final AtomicReference<BigInteger[]> lastFactors = new AtomicReference<>();

    public void service(ServletRequest req, ServletResponse resp) {
        BigInteger i = extractFromRequest(req);
        if i.equals(lastNumber.get())) {
            encodeIntoResponse(resp, lastFactors.get());
        }
        else {
            BigInteger[] factors = factor(i);
            lastNumber.set(i);
            lastFactors.set(factors);
            encodeIntoResponse(resp, factors);
        }
    }
}

Instance variables are thread safe, then why the whole class is not thread safe ?

Upvotes: 1

Views: 217

Answers (1)

Andy Turner
Andy Turner

Reputation: 140299

It's not thread safe because you don't always get the right answer when multiple threads call the code.

Let's say that lastNumber=1 and lastFactors=factors(1). In the one-thread case, where the thread calls with i=1:

T1: if (lastNumber.get().equals(1)) { // true
T1:   encodeIntoResponse(resp, lastFactors.get());

Fine, this is the expected result. But consider a multi-threaded case, where the actions within each thread takes place in the same order, but can arbitrarily interleave. One such interleaving is (where i=1 and i=2 for the two threads respectively):

T1: if (lastNumber.get().equals(1)) { // true

T2: if (lastNumber.get().equals(2)) { // false
T2: } else {
T2:   lastNumber.set(2);
T2:   lastFactors.set(factors(2));

T1:   encodeIntoResponse(resp, lastFactors.get()); // oops! You wrote the factors(2), not factors(1).

The problem is that you're not getting and setting the AtomicReferences atomically: that is, there is nothing to stop another thread sneaking in and changing the values (of one or either) between the get and the set.

In general, whilst individual calls to methods on an AtomicReference are atomic, multiple calls are not (and they definitely aren't atomic between instances of AtomicReference). So, if you ever find yourself writing code like:

if (/* some condition with ref.get() */) {
  /* some statement with ref.set() */
}

then you probably aren't using AtomicReference correctly (or, at least, it's not thread-safe).

To fix this, you need something that can be read and set atomically. For example, create a simple class to hold both:

class Holder {
  // Add a ctor to initialize these.
  final BigInteger number;
  final BigInteger[] factors;
}

Then store this in a single AtomicReference, and use updateAndGet:

BigInteger[] factors = holderRef.updateAndGet(h -> {
  if (h != null && h.number.equals(i)) {
    return h;
  }
  return new Holder(i, factor(i));
}).factors;
encodeIntoResponse(resp, factors);

Upon reflection, updateAndGet isn't necessarily the right way to do this. If factors sometimes takes a long time to compute, then a long-time computation might get done many times, because lots of other shorter-time computations preempt it, so the update function keeps having to be called.

Instead, you can just always set the reference if you had to recompute it:

Holder h = holderRef.get();
if (h == null || !h.number.equals(i)) {
  h = new Holder(i, factors(i));
  holderRef.set(h);
}
return h.factors;

This may seem to violate what I said previously, in that separate calls to holderRef are not atomic, and thus not thread-safe.

It's a bit more nuanced, however: my first paragraph states that the lack of thread safety in the original code stems from the fact that you might get the factors for the wrong input. This problem doesn't occur here: you either get the holder for the right number (and hence the factors for the right number), or you compute the factors for the input.

The issue arises in what this holder is actually meant to be storing: the "last" number/factors is rather hard to define in terms of multithreading. When are you measuring "last-ness" from? The most recent call to start? The most recent call to finish? Other?

This code simply stores "a" previously computed value, without attempting to nail down this ambiguity.

Upvotes: 4

Related Questions