3

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

Nathan Hughes
  • 94,330
  • 19
  • 181
  • 276
David SCHERRER
  • 73
  • 8
  • I assume you mean doAction to be a synchronized method? (It's not in your snippet.) – Will Jul 13 '12 at 14:19
  • Do you have `synchronized` keyword on the 3 methods: addDataIn... , etc.? – nhahtdh Jul 13 '12 at 14:19
  • indeed - corrected. The other methods are private and are not direclty synchronized themselves – David SCHERRER Jul 13 '12 at 14:21
  • 1
    this means that your threads will be able to add,get or remove only one thread at the time, not the best solution i have to say. – memo Jul 13 '12 at 14:25
  • I don't think this is a good idea to make cache methods unsynchronized and make entry point synchronized because in case you can perform anly one operation – amicngh Jul 13 '12 at 14:27
  • As I precised. There is not much use of the map but it has to remain safe as shared ressource. I might change to a synchronized Map implementation in the future. – David SCHERRER Jul 13 '12 at 14:38

8 Answers8

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

memo
  • 441
  • 2
  • 6
1

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

Mohan
  • 1,850
  • 1
  • 19
  • 42
  • sorry, didn't noticed the synchronized keyword – Mohan Jul 13 '12 at 14:23
  • To be more specific, even if you move the synchronized keyword away from doAction() to specific methods like removeDataFromMyMap(), it won't be sufficient. The above will give you some small performance penalty(micro second level), since you are synchronizing at the entry public method level, but will guarentee you thread safety. Once I benchmarked the performance of ConcurrentHashMap vs the synchronized{hashmap}. 70th percentile came around 15 micros - ConcurrentHashMap is faster when multiple threads access it rather than synchronizing the hashmap like the way you did. – Mohan Jul 13 '12 at 14:27
  • thanks, that was the point. I just added a note - yes this is the only entry point – David SCHERRER Jul 13 '12 at 14:32
1

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.

amicngh
  • 7,831
  • 3
  • 35
  • 54
1

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.)

Nathan Hughes
  • 94,330
  • 19
  • 181
  • 276
0

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

nhahtdh
  • 55,989
  • 15
  • 126
  • 162
0

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.

dvberkel
  • 663
  • 4
  • 16
0

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

Arvik
  • 2,524
  • 2
  • 15
  • 10
0

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);
  }
}
Matt
  • 11,523
  • 2
  • 23
  • 33