Why is synchronization needed here?

Question

Hi,

I am trying to plug some shameful gaps in my Java threading knowledge, and I'm reading Java Concurrency in Practice by Brian Goetz et al (highly recommended BTW), and one of the early examples in the book leaves me with a question. In the following code, I totally understand why synchronization is needed when updating the hits and cacheHits member variables, but why is it needed for the getHits method when just reading the hits variable?

Example code from chapter 2:

public class CachedFactorizer extends GenericServlet implements Servlet {
  private BigInteger lastNumber;
  private BigInteger[] lastFactors;
  private long hits;
  private long cacheHits;

public synchronized long getHits() {
    return hits;
}

public synchronized double getCacheHitRatio() {
    return (double) cacheHits / (double) hits;
}

public void service(ServletRequest req, ServletResponse resp) {
    BigInteger i = extractFromRequest(req);
    BigInteger[] factors = null;
    synchronized (this) {
        ++hits;
        if (i.equals(lastNumber)) {
            ++cacheHits;
            factors = lastFactors.clone();
        }
    }
    if (factors == null) {
        factors = factor(i);
        synchronized (this) {
            lastNumber = i;
            lastFactors = factors.clone();
        }
    }
    encodeIntoResponse(resp, factors);
}...

I have a feeling it is to do with atomicity, monitors and locks, but I don't fully understand those, so please could someone out there explain a little deeper?

Thanks in advance...

James

Answer 1

Because otherwise you may see a stale value of hits. Volatile would have also worked. This is relevant http://www.ibm.com/developerworks/java/library/j-jtp04186/index.html

Answer 2

Basic rule of thumb is that when you need synchronized access to a variable, you always need it for both read and write. Else there is a chance that the value updated by one thread will never be visible to the other threads ...

Answer 3

The idea of Monitors and locks is to prevent two seperate threads from simultaneously attempting to either access or modify some specific resource that is used by both threads. You are effectively asking "But why lock when I'm only reading the data?" - and the answer is because if you don't, someone else could modify it at the same time, and you would not be aware of the new value - which depending on the logic of your program might cause a number of difficult to catch bugs as your program executes - commonly known as race conditions...

When working with threads, code defensively, tread (and indeed thread) carefully.

Answer 4

Because hits is not marked as volatile. If you are accessing a variable that can be changed to more than one thread, like this, you have to mark it as volatile or access it in a synchronized block.

EDIT: As said in here and exemplified in the Effective Java book, if you don't access it through a synchronized block you could not see a value that was changed by another thread. Please note that this is happening because that variable is not final and it is not one of the wrappers in the concurrency package. If it was final and one of those variables, then the synchronized block wouldn't be necessary.

... another option would be to use an AtomicLong, making it final. That variable wouldn't have to be volatile because the variable is not change, only its contents, which is managed by the AtomicLong class.

Answer 5

Apart from the fact that a thread's cache view of non-volatile variables may stay non-updated indefinitely when there is no synchronization:

read/write access to a long or double is NOT guaranteed to be atomic!

You could theoretically end up seeing a value where only the first or last 4 bytes are updated. However, volatile solves this problem as well.

Answer 6

There are multiple potential issues here. Michael has pointed out one big one (non-atomicity of long stores) but there's another. In the absence of a 'happens-before' relationship (which is provided between the release of and the acquisition of a lock, e.g. in a synchronized block), the writes can be seen out of order.

Notice that the line ++hits comes before the ++cacheHits in service(). In the absence of synchronized, the JVM is quite entitled to reorder those instructions in a way which may seem confusing to other threads. For example, it may reorder the ++cacheHits before the ++hits, or it may make the increased value of cacheHits visible to other threads before the increased value of hits (in this case the distinction isn't important, because the result could be the same). Imagine a reordering, starting from a clean cache, which results in the following interleaving:

Thread 1                  Thread 2
---------------           ----------------
++cacheHits (reordered)
  cacheHits=1, hits=0
                          read hits (as 0)
++hits
  cacheHits=1, hits=1
                          read cacheHits (as 1)

                          calculate 1 / 0 (= epic fail)

You won't get the result you're expecting, that's for sure.

Mind you, that's easy to debug. You could have 1000 service() calls, and then the reading thread sees cacheHits as 500 and hits as 1. A 50,000% cache hit ratio is probably less obvious and even more confusing to the poor debugger.

Synchronizing the read sets up a happens-before relationship so this won't happen, and the locking then provides the other advantages that others mention.

Answer 7

If you are concerned about the performance of the code (which is my guess), you should know that (1) stability is preferred over speed in concurrent applications, and (2) other people like you in Sun had the same concern and thus devised this:

ReentrantReadWriteLock