Java concurrency scenario -- do I need synchronization or not?

Question

Here's the deal. I have a hash map containing data I call "program codes", it lives in an object, like so:

Class Metadata
{
    private HashMap validProgramCodes;
    public HashMap getValidProgramCodes() { return validProgramCodes; }
    public void setValidProgramCodes(HashMap h) { validProgramCodes = h; }
}

I have lots and lots of reader threads each of which will call getValidProgramCodes() once and then use that hashmap as a read-only resource.

So far so good. Here's where we get interesting.

I want to put in a timer which every so often generates a new list of valid program codes (never mind how), and calls setValidProgramCodes.

My theory -- which I need help to validate -- is that I can continue using the code as is, without putting in explicit synchronization. It goes like this: At the time that validProgramCodes are updated, the value of validProgramCodes is always good -- it is a pointer to either the new or the old hashmap. This is the assumption upon which everything hinges. A reader who has the old hashmap is okay; he can continue to use the old value, as it will not be garbage collected until he releases it. Each reader is transient; it will die soon and be replaced by a new one who will pick up the new value.

Does this hold water? My main goal is to avoid costly synchronization and blocking in the overwhelming majority of cases where no update is happening. We only update once per hour or so, and readers are constantly flickering in and out.

Answer 1

I think it's risky. Threading results in all kinds of subtly issues that are a giant pain to debug. You might want to look at FastHashMap, which is intended for read-only threading cases like this.

At the least, I'd also declare validProgramCodes to be volatile so that the reference won't get optimized into a register or something.

Answer 2

The assignment will work as long as you're not concerned about reading stale values, and as long as you can guarantee that your hashmap is properly populated on initialization. You should at the least create the hashMap with Collections.unmodifiableMap on the Hashmap to guarantee that your readers won't be changing/deleting objects from the map, and to avoid multiple threads stepping on each others toes and invalidating iterators when other threads destroy.

( writer above is right about the volatile, should've seen that)

Answer 3

I don't know much about threading but I am trying to think together with you, this is more like a question to you and the others.

1. What can happen if the assignment operation is "half-done", and then another thread tried to read the reference?

2. If you put a lock around the assignment block, does this mean that the readers will have to wait for a little sec? But thus will only happen once in an hour right?

Answer 4

I think your assumptions are correct. The only thing I would do is set the validProgramCodes volatile.

private volatile HashMap validProgramCodes;

This way, when you update the "pointer" of validProgramCodes you guaranty that all threads access the same latest HasMap "pointer" because they don't rely on local thread cache and go directly to memory.

Answer 5

If I read the JLS correctly (no guarantees there!), accesses to references are always atomic, period. See Section 17.7.

So, if the access to a reference is always atomic and it doesn't matter what instance of the returned Hashmap the threads see, you should be OK. You won't see partial writes to the reference, ever.

---

Edit: After review of the discussion in the comments below and other answers, here are references/quotes from

Doug Lea's book (Concurrent Programming in Java, 2nd Ed), p 94, section 2.2.7.2 Visibility, item #3: "

The first time a thread access a field of an object, it sees either the initial value of the field or the value since written by some other thread."

On p. 94, Lea goes on to describe risks associated with this approach:

The memory model guarantees that, given the eventual occurrence of the above operations, a particular update to a particular field made by one thread will eventually be visible to another. But eventually can be an arbitrarily long time.

So when it absolutely, positively, must be visible to any calling thread, volatile or some other synchronization barrier is required, especially in long running threads or threads that access the value in a loop (as Lea says).

However, in the case where there is a short lived thread, as implied by the question, with new threads for new readers and it does not impact the application to read stale data, synchronization is not required.

---

@erickson's answer is the safest in this situation, guaranteeing that other threads will see the changes to the HashMap reference as they occur. I'd suggest following that advice simply to avoid the confusion over the requirements and implementation that resulted in the "down votes" on this answer and the discussion below.

I'm not deleting the answer in the hope that it will be useful. I'm not looking for the "Peer Pressure" badge... ;-)

Answer 6

I have to go with Ken G and Bill the Lizard on this one. Your assumptions will work fine. The only addition I would recommend is that the HashMap is made unmodifiable (as has been suggested).

Also, depending on how the reader threads are using your code table, marking it as volatile may actually cause logical problems, as successive access to the map may give an inconsistent view of the data stored within it. That is, getting some old and some new values from consecutive reads on the same thread may be logically inconsistent.

Answer 7

No, by the Java Memory Model (JMM), this is not thread-safe.

There is no happens-before relation between writing and reading the HashMap implementation objects. So, although the writer thread appears to write out the object first and then the reference, a reader thread may not see the same order.

As also mentioned there is no guarantee that the reaer thread will ever see the new value. In practice with current compilers on existing hardware the value should get updated, unless the loop body is sufficienly small that it can be sufficiently inlined.

So, making the reference volatile is adequate under the new JMM. It is unlikely to make a substantial difference to system performance.

The moral of this story: Threading is difficult. Don't try to be clever, because sometimes (may be not on your test system) you wont be clever enough.

Answer 8

No, the code example is not safe, because there is no safe publication of any new HashMap instances. Without any synchronization, there is a possibility that a reader thread will see a partially initialized HashMap.

Check out @erickson's explanation under "Reordering" in his answer. Also I can't recommend Brian Goetz's book Java Concurrency in Practice enough!

Whether or not it is okay with you that reader threads might see old (stale) HashMap references, or might even never see a new reference, is beside the point. The worst thing that can happen is that a reader thread might obtain reference to and attempt to access a HashMap instance that is not yet initialized and not ready to be accessed.

Answer 9

As others have already noted, this is not safe and you shouldn't do this. You need either volatile or synchronized here to force other threads to see the change.

What hasn't been mentioned is that synchronized and especially volatile are probably a lot faster than you think. If it's actually a performance bottleneck in your app, then I'll eat this web page.

Another option (probably slower than volatile, but YMMV) is to use a ReentrantReadWriteLock to protect access so that multiple concurrent readers can read it. And if that's still a performance bottleneck, I'll eat this whole web site.

  public class Metadata
  {
    private HashMap validProgramCodes;
    private ReadWriteLock lock = new ReentrantReadWriteLock();

    public HashMap getValidProgramCodes() { 
      lock.readLock().lock();
      try {
        return validProgramCodes; 
      } finally {
        lock.readLock().unlock();
      }
    }

    public void setValidProgramCodes(HashMap h) { 
      lock.writeLock().lock();
      try {
        validProgramCodes = h; 
      } finally {
        lock.writeLock().unlock();
      }
    }
  }

Answer 10

While this is not the best solution for this particular problem (erickson's idea of a new unmodifiableMap is), I'd like to take a moment to mention the java.util.concurrent.ConcurrentHashMap class introduced in Java 5, a version of HashMap specifically built with concurrency in mind. This construct does not block on reads.