views:

66

answers:

4

The question came up when I saw this code:

private static volatile ConcurrentHashMap<String, String> cMap = null;
static {
    cMap = new ConcurrentHashMap<String, String>();
}

To me it looks like the volatile there is redundant as the container is ConcurrentHashMap which according the JavaDoc already has synchronized puts, DUH, the class that uses the cMap only instantiates it once and doesn't have any methods of setting or getting it.

The only thing I see volatile providing here is that if I would be setting the cMap to reference a new object in near future, those reads and writes would be synchronized.

Am I missing something?

+1  A: 

You would declare cMap volatile only when its value changes. Declaring it volatile says nothing about the objects held in the map.

If cMap changes all threads will need to see the new up to date value of the CHM. That being said, I would highly recommend cMap being final. A non final static variable can be dangerous.

John V.
+6  A: 

The volatile modifier doesn't have anything to do with the class involved - it's only to do with the variable cMap. It only affects how a thread fetches or changes the value of that variable. By the time you've got as far as invoking methods on the referenced object, you've gone beyond the bailiwick of volatile.

As you say, it basically makes sure that all threads would be guaranteed to see changes to the cMap value (i.e. making it refer to a different map).

That may be a good idea - or it may not, depending on what the rest of the code does. If you could make it final for example, you wouldn't need it to be volatile...

Jon Skeet
Yeah, making it final makes sense.
Bleadof
+2  A: 

Declaring the cMap reference as volatile ensures that its initialized value is visible to all threads. AFAIK without this, it is not guaranteed, i.e. some threads might see a null reference instead of the reference to a properly initialized map object.

[Update] As @irreputable pointed out (and as is explained in Java Concurrency in Practice, section 3.5.3), I was wrong with the above statement: static initializers are indeed executed by the JVM at class initialization time, guarded by internal synchronization of the JVM. So volatile is not necessary.[/Update]

OTOH declaring it final (and initializing it straight away, not in a separate static block) would guarantee visibility too.

Note that the reference being volatile or not has nothing to do with the class of the variable it refers to. Even if the referred class is threadsafe, the reference itself may not be.

Péter Török
I think you are wrong, see my answer.
irreputable
@irreputable, oops, you are right. My bad :-( Fixed and +1 for you :-)
Péter Török
+5  A: 

unless the variable is re-assigned later, the volatile is totally unnecessary.

any writes occurred in the static initializer are visible to any code using the class (i.e. when a static method/field is accessed, when a constructor is invoked)

without that guarantee, we are in deep trouble. millions lines of code would be wrong.

see JLS3 $12.4.2:

The procedure for initializing a class or interface is then as follows:

  1. Synchronize (§14.19) on the Class object that represents the class or interface to be initialized.
irreputable