Is this usage of synchronizedMap thread safe?

I have a singleton class, that has a map which can be accessed by multiple threads at the same time. Could somebody please check the code below and tell me if its thread safe? (note: I dont plan to use ConcurrentHashMap, and the printMap method is called only seldom.)

    public  class MySingleton{

      private Map<String,String>  cache = Collections.synchronizedMap(
new LinkedHashMap<String,String>());

      public String getValue(String key){
         return cache.get(key)
      }

      pu开发者_StackOverflow社区blic void setValue(String key, String value){
         cache.put(key, value);
      }

      public void printMap(){
          synchronized(cache){

              for(Entry<String,String> entry: cache.entrySet()){
                    println('key: '+entry.getKey()+', value: ' + value);

               }
          }

      }
    }

My test is working... but i am doubting if this code is good enough to be called 'thread safe'.

points that I considered:

1. The readValue and putValue methods don't need to have a 'synchronized' block since i am using a synchronizedMap

2. printMap should have the synchronized block, since the javadoc for says that we should synchronize the Map instance before each iteration. http://download.oracle.com/javase/1.5.0/docs/api/java/util/Collections.html#synchronizedMap%28java.util.Map%29

Any help is appreciated.

---

Yes, that's okay. The key thing is that while you're iterating, nothing will be able to modify the map because cache.put will end up synchronizing on cache anyway.

Personally I'd rather make that explicit, by using a "normal" hashmap and synchronizing on the same object (whether the map or something else) from all three methods - but what you've got should be fine.

(Alternatively, you could use ConcurrentHashMap to start with. It's worth at least looking at that.)

---

Yes it is thread safe. Each access to the cache is synchronized (by the synchronizedMap for get and set and by an explicit sync block for the printMap)

---

Yes, this class is thread-safe.

Though note that even a thread-safe class requires safe publication to be used really safely (without safe publication nothing guarantees that other threads can't see cache in non-initialized state, i.e. null).

But in this case you can eliminate a need in safe publication by making your class immutable (final keyword guarantees that other threads can't see null in cache):

private final Map<String,String>  cache = Collections.synchronizedMap( new LinkedHashMap<String,String>());