Is a java synchronized method entry point thread safe enough?

Question

I have a Singleton class handling a kind of cache with different objects in a Hashmap. (The format of a key is directly linked to the type of object stored in the map - hence the map is of )

Three different actions are possible on the map : add, get, remove.

I secured the access to the map by using a public entry point method (no intense access) :

public synchronized Object doAction(String actionType, String key, Object data){
  Object myObj = null;
  if (actionType.equalsIgnorecase("ADD"){
    addDataToMyMap(key,data);
  } else if (actionType.equalsIgnorecase("GET"){
    myObj = getDataFromMyMap(key);
  } else if (actionType.equalsIgnorecase("REM"){  
    removeDataFromMyMap(key);      
  }
  return myObj;
}

Notes:

The map is private. Methods addDataToMyMap(), getDataFromMyMap() and removeDataFromMyMap() are private. Only the entry point method is public and nothing else except the static getInstance() of the class itself.

Do you confirm it is thread safe for concurrent access to the map since there is no other way to use map but through that method ?

If it is safge for a Map, I guess this principle could be applied to any other kind of shared ressource.

Many thanks in advance for your answers.

David

Answer 1

I would need to see your implementation of your methods, but it could be enough. BUT i would recommend you to use a Map from the Collection API of java then you wouldnt need to synchronize your method unless your sharing some other instance.

read this: http://www.java-examples.com/get-synchronized-map-java-hashmap-example

Answer 2

Yes your class will be thread safe as long as the only entry point is doAction.

Answer 3

If your cache class has private HashMap and you have three methods and all are public synchronized and not static and if you don't have any other public instance variable then i think your cache is thread-safe.

Better to post your code.

Answer 4

This is entirely safe. As long as all the threads are accessing it using a common lock, which in this case is the Object, then it's thread-safe. (Other answers may be more performant but your implementation is safe.)

Answer 5

You can use Collections.synchronizedMap to synchronize access to the Map.

Answer 6

As is it is hard to determine if the code is thread safe. Important information missing from your example are:

1. Are the methods public
2. Are the methods synchronized
3. It the map only accessed through the methods

I would advice you to look into synchronization to get a grasp of the problems and how to tackle them. Exploring the ConcurrentHashMap class would give further information about your problem.

Answer 7

You should use ConcurrentHashMap. It offers better throughput than synchronized doAction and better thread safety than Collections.synchronizedMap().

Answer 8

This depends on your code. As someone else stated, you can use Collections.synchronizedMap. However, this only synchronizes the individual method calls on the map. So if:

map.get(key);
map.put(key,value);

Are executed at the same time in two different threads, one will block until the other exits. However, if your critical section is larger than the single call into the map:

SomeExpensiveObject value = map.get(key);
if (value == null) {
   value = new SomeExpensiveObject();
   map.put(key,value);
}

Now let's assume the key is not present. The first thread executes, and gets a null value back. The scheduler yields that thread, and runs thread 2, which also gets back a null value. It constructs the new object and puts it in the map. Then thread 1 resumes and does the same, since it still has a null value.

This is where you'd want a larger synchronization block around your critical section

SomeExpensiveObject value = null;

synchronized (map) {
  value = map.get(key);
  if (value == null) {
     value = new SomeExpensiveObject();
     map.put(key,value);
  }
}