Synchronization on boxed primitive

Question

I am new to multithreaded programming. So I need some help to this issue. I get a findbugs bug with synchronization on a boxed primitive:

http://findbugs.sourceforge.net/bugDescriptions.html#DL_SYNCHRONIZATION_ON_BOXED_PRIMITIVE

I tried some solutions on this site but it doesn't work as I expected. Sometimes I get a similar error from findbugs.

My code needs a lock on a id which I pass to a constructor, here is some pseudocode:

public class MyClass{
   public MyClass(long id){
      synchronized(id){
         // do some stuff
      }    
   }
}

The problem is, that only threads with the same id should block on the synchronized block. Threads with different ids should work simultaneously.

I also tried something like this, but it doesn't work for me:

public class MyClass{

   private static final ConcurrentHashMap<Long, Object> myHashMap = new ConcurrentHashMap<Long, Object>();

   public MyClass(long id){

      Object object = getObject(id);

      synchronized(object){
         // do some stuff
      }    
   }

   private Object getObject(long id){
      if(!myHashMap.contains(id)){
         writeObject(id);
      }
      return myHashMap.get(id);
   }

   private synchronized void writeObject(long id){
      if(!myHashMap.contains(id)){
          myHashMap.put(id, new Object());
      }
   }
}   

In the second example you see, I tried to put an object per id in the hashmap, but with an unit test I realized, that threads with same id get in the synchronized block. But they should not do. I will be very thankful if someone has another solution or how to handle these problems.

Answer 1

Good for you that you wrote an unit test! For future reference: You want to test all kinds of edge values, which means for numbers at least 0, -1, 1, MAX_VALUE, MIN_VALUE - that would've caught the second bug you missed :-)

The problem with your code is that: synchronized(l) {} translates to: synchronized(Long.valueOf(l)){}. valueOf caches Longs in the range -128, 127 but even this is optional (the JLS only requires it for integers surprisingly!). Consequently as soon as your ids are larger than 127 your whole scheme falls apart.

Your second method is the way to go, but you cannot just make the method synchronized - that will just synchronize on this, so doesn't guarantee atomicity for the static map.

Instead do something like this:

Object newLock = new Object();
Object oldLock = map.putIfAbsent(id, newLock);
Object lock = oldLock != null ? oldLock : newLock;
synchronized(lock) { 

}