Java multi-threading atomic reference assignment

I have a cache which I implemented using a simeple HashMap. like -

HashMap<String,String> cache = new HashMap<String,String>();

This cache is used most of the time to read values from it. I have another method which reloads the cache and inside of this method I basically create a new cache and then assign the reference. As I understand assignment of object reference is Atomic in Java.

public class myClass {
     private HashMap<String,String> cache = null;
    public void init() {
       refreshCache();
    }
    // this method can be called occasionally to update the cache.
    public void refreshCache() {
        HashMap<String,String> newcache = new HashMap<String,String&开发者_JAVA百科gt;();
       // code to fill up the new cache
       // and then finally
       cache = newcache; //assign the old cache to the new one in Atomic way
    }
}

I understand that if I do not declare cache as volatile, other threads will not be able to see the changes but it is not time critical for my use case to propagate the change in cache to other threads and they can continue to work with old cache for extended time.

Do you see any threading issue? Consider that many threads are reading from the cache and only at times the cache is reloaded.

EDIT- My main confusion is I do not have to use AtomicReference here as the assignment operation itself is atomic?

EDIT - I understand that to make the ordering proper, I should mark cache as volatile. But If refreshCache method is marked as synchronized, I do not have to make cache as volatile, as Synchronized block will take care of ordering as well as visibility?

It is not safe without a proper memory barrier.

One would think that the assignment of cache (cache = newCache) would happen after the steps to populate the cache. However, other threads may suffer from reordering of these statements so that the assignment may appear to happen before populating the cache. Thus, it is possible to grab the new cache before it's fully constructed or even worse see a ConcurrentModificationException.

You need to enforce the happens-before relationship to prevent this reordering, and declaring the cache as volatile would achieve that.

You should mark the cache as volatile.

While you note that other threads may continue using a stale cache for "a long time" you should note that, without a synchronization edge, they are likely to continue using the stale cache forever. That's probably not the desired behavior.

In order of preference (mostly due to readability):

• Update the field in a synchronized method
• Use AtomicReference<Map<>>
• Use volatile

See also this question.

What about CopyOnWrite.. collections:

java.util.concurrent.CopyOnWriteArraySet
java.util.concurrent.CopyOnWriteArraySet
and org.apache.mina.util.CopyOnWriteMap

They can be a good match in your case and they are thread-safe.

Seems to be OK. Be sure refreshCache is not called too frequently or mark as synchronized.