Java assignment issues - Is this atomic?

Question

I've got some questions about Java's assigment.

• Strings

I've got a class:

public class Test {
 private String s;

 public synchronized void setS(String str){
  s = s + " - " + str;
 }

 public String getS(){
  return s;
 }
}

I'm using "synchronized" in my setter, and avoiding it in my getter, because in my app, there are a tons of data gettings, and very few settings. Settings must be synchronized to avoid inconsistency. My question is: is getting and setting a variable atomic? I mean, in a multithreaded environment, Thread1 is about to set variable s, while Thread2 is about to get "s". Is there any way the getter method could get something different than the s's old value or the s's new value (suppose we've got only two threads)? In my app it is not a problem to get the new value, and it is not a problem to get the old one. But could I get something else?

• What about HashMap's getting and putting?

considering this:

    public class Test {
        private Map<Integer, String> map = Collections.synchronizedMap(new HashMap<Integer, String>());

        public synchronized void setMapElement(Integer key, String value){
         map.put(key, value);
        }

        public String getValue(Integer key){
         return map.get(key);
        }
}

Is putting and getting atomic? How does HashMap handle putting an element into it? Does it first remove the old value and put the now one? Could I get other than the old value or the new value?

Thanks in advance!

Answer 1

In the first case, String happens to be safe for unsafe publication (in the "new" Java Memory Model (JMM)), so this is okay.

Not being volatile there is theoretically some issue with not having an up to date value, but then the meaning of up to date is not clear. You could replace the lock with a compare-sand-swap (CAS) loop, but that probably wouldn't give you much performance gain whether the lock was likely to be contended or not.

In the case of HashMap, an unsynchronized map is not safe to read if there is another thread writing to it, even a single writer thread. In fact, this has been found to lead to infinite loops on production systems running popular software. The code in the question actually uses two locks for the map, which is over the top (although you'd need an explicit hold of the same lock if using an iterator). Not being final does stop the containing class from being safe for unsafe publication. If map was volatile and you created a new map for every put, then that could be made to be safe without synchronisation on the get.

Answer 2

Rather than wrapping your HashMap in something to make it synchronized, consider using a java.util.concurrency.ConcurrentHashMap instead.

This is an updated version of HashMap that guarantees "Retrievals reflect the results of the most recently completed update operations holding upon their onset.".