0

Say I have a Resource class which holds a Map type resources object:

public class Resource {
   private Map<Integer, MyResource> resources = new HashMap<Integer, MyResource>();

   public void addResource(MyResource r){
     synchronized(resources){
        resources.put(r.getId(), r);
     }
   }

   public void removeResource(int id){
      synchronized(resources){
            resources.remove(id);
      }
   }

}

Since multiple threads could access the resources, so I always use synchronized block for adding new resource & removing existing resource like the code shows above.

Now, I would also like to add a function to get all the current resources, my question is very simple, that's should I use synchronized block too to return all the current resources or is it worthless to use synchronized block ?

public Map<Integer, MyResource> getResources(){
     return resources;
}

Is it enough to use above code to return current resources, or is it still beneficial to use synchronized block as below:

public Map<Integer, MyResource> getResources(){
   synchronized(resources){
       return resources;
   }
}

Personally, I feel that it doesn't worth to use synchronized block for getting resources since it always return the current moment resources. But I am not sure. So, it would be nice if you could explain to me the reason whether to use synchronized block or not, thanks.

Mellon
  • 37,586
  • 78
  • 186
  • 264
  • pl. take a look this may help you. [synchronization using collections][1] [1]: http://stackoverflow.com/questions/567068/java-synchronized-block-vs-collections-synchronizedmap – user1918096 Dec 05 '13 at 12:24

5 Answers5

3

Is it enough to use above code to return current resources, or is it still beneficial to use synchronized block as below

Neither is enough. It matters little whether you return the reference to the shared map within or without the synchronized block: you must do all the reading actions within a synchronized block. In fact, you should never allow the reference to the map escape your object because then it will be on the loose, with no enforceable synchronization policy.

Marko Topolnik
  • 195,646
  • 29
  • 319
  • 436
1

If you just return resources it means that anyone calling the method will get full and unsynchronized access to it. That is, the caller can add and remove items to the map without telling you and without synchronizing access with other threads. This is probably a bad idea.

The safest thing you can do is create a defensive copy while holding the lock on resources:

public Map<Integer, MyResource> getResources(){
   synchronized(resources){
       return new HashMap<Integer, MyResource>(resources);
   }
}

Update: An alterative is using Collections.synchronizedMap ConcurrentHashMap to ensure all accesses are thread safe, and then return an unmodifiable view so that code calling getResources cannot modify the map you use internally.

Update 2: Don't use Collections.synchronizedMap for this: iterating on the map would require synchronizing on it, and if getResources returns an unmodifiable view there's no way the calling code can do that.

// Don't use synchronizedMap
// private Map<Integer, MyResource> resources = Collections.synchronizedMap(new HashMap<Integer, MyResource>());

private Map<Integer, MyResource> resources = new ConcurrentHashMap<>();

public void addResource(MyResource r) {
    resources.put(r.getId(), r);
}

public void removeResource(int id) {
    resources.remove(id);
}

public Map<Integer, MyResource> getResources() {
    return Collections.unmodifiableMap(resources);
}
Joni
  • 108,737
  • 14
  • 143
  • 193
  • This is safe, but potentially a waste of resources. – Marko Topolnik Dec 05 '13 at 12:25
  • True, using a thread safe map and returning and unmodifiable view like in the edit may be more resource-friendly. – Joni Dec 05 '13 at 12:33
  • `synchronizedMap` isn't reall thread-safe so I wouldn't recommend it. The most important operation, iteration, is not thread-safe on it. – Marko Topolnik Dec 05 '13 at 12:39
  • You are right Marko, with `synchronizedMap` anything that takes the key, entry or value sets and calls `iterator` on them would have to be protected with a synchronized block. And there's no way to do that if `getResources` returns an unmodifiable view and not the original. Is there a safe solution that doesn't imply copying the entire map? – Joni Dec 05 '13 at 13:55
  • 1
    The best choice would be the `ConcurrentHashMap`, which will consistently, even if not completely deterministically, walk through the entries as of "some point at or since the creation of the iterator/enumeration". – Marko Topolnik Dec 05 '13 at 14:00
  • 1
    However my real advice would be to carefully think through what you actually need from the map and design the API around that need, instead of ham-handedly returning a reference to the whole map. – Marko Topolnik Dec 05 '13 at 14:01
0

Since you have to only read the data then you can use simple method also which will be not synchronized .Since the only possibility of dirty data being in the map from remove method since you have already defined it as synchronized at a time only single thread can access it But i think that if the reaDall method will not be synchronized then at the same time two thread can call the two method ie one will call the remove and one can all the read method in that case the dirty data can be there in your object.So it is better if you use synchronized method

Deepak
  • 2,287
  • 1
  • 23
  • 30
0

I think that you should use synchonized blocks for putting new keys. All in all simply use ConcurrentHashMap as map implementation in concurrent environement.

Antoniossss
  • 31,590
  • 6
  • 57
  • 99
  • Hi, thanks but I would like to get some answer based on my current scenario instead of changing the resource type to ConcurrentHashMap. Besides, what I am asking is not about putting new keys, but about getting the resources from thread-safe perspective. – Mellon Dec 05 '13 at 12:18
  • Than synchronize `put` action. No need for read synchronization. – Antoniossss Dec 05 '13 at 12:19
0
public Map<Integer, MyResource> getResources(){
     return resources;
}


public Map<Integer, MyResource> getResources(){
   synchronized(resources){
       return resources;
   }
}

both the ways are not immune of issues. As you are sending the original Map to your consumer so they can change it which will impact your class invariant. You use defensive copy, or use immutable map i.e.Collections.unmodifiableMap.

Trying
  • 14,004
  • 9
  • 70
  • 110