0

I have to search for an object: first in a BlockingQueue, and if it is not there then I need to search in a ConcurrentHashMap and need to do some operation. This needs to be thread-safe.

Is the code below OK? Does synchronizing on the ConcurrentHashMap work as expected?

synchronized(blockingQueue){
   if(!blockingQueue.contains(element)) {        
      synchronized(concurrentHashMap) {    
             //do something
      }
   }
}
Dariusz
  • 21,561
  • 9
  • 74
  • 114
Jitendra kumar jha
  • 243
  • 1
  • 2
  • 12

4 Answers4

2

First of all, synchronizing like in your example may not do what you expect. You would have to check the implementations of both those collections and check if they synchronize on themselves and not any other internal object.

If you need to synchronize access like this, I think that using synchronized collections is a bad idea. Obviously, your critical sections are more complex than simple read/write operations. Consider using custom lock for operations, like this:

final Object lock = new Object();

public void addDataToHashMap(Object param, Object val) {
  synchronized(lock) {
    concurrentHashMap.put(param, val);
  }
}

public void performComplexOperations() {
  synchronized (lock) {
    if (!blockingQueue.contains(element)) {        
      processSomeData(concurrentHashMap);
    }
  }
}
Bex
  • 2,905
  • 2
  • 33
  • 36
Dariusz
  • 21,561
  • 9
  • 74
  • 114
  • I agree...creating custom lock is more appropriate rather then using synchronized collections itself – Rachel Aug 20 '13 at 16:29
1

An important clarification that has not been included in the previous answers is that synchronized(concurrentHashMap) will not lock the hash map, and `synchronized(blockingQueue) will not lock the queue, and they can continue being updated in some other thread that is not synchronized on the same object(s).

Quoting the javadoc for ConcurrentHashMap:

However, even though all operations are thread-safe, retrieval operations do not entail locking, and there is not any support for locking the entire table in a way that prevents all access.

It would be helpful to know what //do something is supposed to do to give a better answer.

Bex
  • 2,905
  • 2
  • 33
  • 36
0

I would say that best way to do is to wrap this logic inside a a separate common function and synchronize it on a common lock (say instance of the utility class where you put this function) like below :

class MyUtil{

    public synchronized Object getMyObject(){    
        //if not in queue then get from map
        //no need to put any lock on queue or map now
    }
}
Bex
  • 2,905
  • 2
  • 33
  • 36
Saurabh
  • 7,894
  • 2
  • 23
  • 31
  • according to the book java concurrency in practice public class ListHelper{ public List list = Collections.synchronizedList(new ArrayList()); .... public synchronized boolean putIfAbsent(E x){ boolean absent = !list.contains(x); if(absent){ list.add(x); return absent; } } } This code is not thread safe, rather we need to take lock on the list itself. so instead use like synchronized(list){ //do things } – Jitendra kumar jha Aug 20 '13 at 08:24
  • you have misunderstood that concept in perspective of current situation. What is written in book is right. However the message of book was that two different synchronized operation separately can't be considered as atomic operation together. To handle this create a synchronized block and performed these two operations inside one critical section – Saurabh Aug 20 '13 at 09:21
  • Rather then synchronizing on method itself, I would prefer using custom locks as @Dariusz mentioned – Rachel Aug 20 '13 at 16:30
0

Could you explain more ? who is accessing those objects blockingQueue and concurrentHashMap. There are threads that only reads / only writes to them ?

Maybe you should take a look at java.util.concurrent.locks.ReadWriteLock.

Radu Toader
  • 1,521
  • 16
  • 23