Synchronizing on an Integer results in NullPointerException

Question

This question is based on Synchronizing on an Integer value.

The solution there seems excellent only there is small problem it does not address the concern how to delete values from ConcurrentHashMap.

So to address that I did below program

import java.util.concurrent.ConcurrentHashMap;

public class Example {

    private final ConcurrentHashMap<Integer, Integer> concurrentHashMap = new ConcurrentHashMap<Integer, Integer>();

    public void doSomething(int i) {
        synchronized (getLockForId(i)) {
            concurrentHashMap.remove(i);
        }
    }

    public Integer getLockForId(int id) {
        concurrentHashMap.putIfAbsent(id, id); // I want to replace these two
                                                // operation with single one
                                                // since it seems the cause of
                                                // NPE
        return concurrentHashMap.get(id);
    }

    public static void main(String[] args) {
        final Example example = new Example();
        new Thread(new Runnable() {
            @Override
            public void run() {
                int i = 0;
                while (true) {
                    example.doSomething(++i);
                }
            }
        }).start();
        new Thread(new Runnable() {
            @Override
            public void run() {
                int i = 0;
                while (true) {
                    example.doSomething(++i);
                }
            }
        }).start();
    }
}

Problem is that it always results in NullPointerException. My first analysis was because I am deleting the value it gets assigned to null so it is causing NullPointerException. So I did below

    Object obj = new Object();
    synchronized (obj) {
        obj = null;
    }

But above does not result in NullPointerException. So My question is why it is throwing NullPointerException in above case?

Even if I do

public Integer getLockForId(int id) {
   return concurrentHashMap.putIfAbsent(id, id); 
}

It still results in NullPointerException because it only returns value when there is one else return null

Trolley

This question is based on Synchronizing on an Integer object not value. – Gumm

Why do you need to remove values from map when you're done? – Vanderhoek

@NikitaBeloglazov Eventually there has to be a point where those need to be removed right? The problem it that point needs to be synchronized. I see no other solution for it then. – Trolley

@Gumm I can not change text of Previous question :) – Trolley

I don't see reasons why you need to remove these keys, actually. Only if you may have about 2^32 different ids during your application lifetime. – Vanderhoek

Answer 1

Well yes, that would throw a NullPointerException. Consider this pattern:

 Thread 1                     Thread 2

 putIfAbsent (puts)
 get (returns non-null)
 acquire monitor
                              putIfAbsent (doesn't put)
 remove (removes value)
                              get (returns null)
                              acquire monitor (bang!)

It's not that the "value get assigned to null" - it's that Map.get returns null if there's no entry for the given key.

It's hard to know what to recommend, as your code really doesn't do anything useful. If you can say what you're trying to achieve in your real code, we can give you better suggestions, potentially.

EDIT: As noted by Nikita, just returning the value of putIfAbsent doesn't work, as that returns the previous value, or null if it was absent - whereas you want the new value for the entry.

I suspect you'll have to synchronize access to the map, basically, to make your getLockId method atomic with respect to the remove operation.

Mythologize

Returning result of putIfAbsent still results in NPE – Trolley

@AmitD: With the code you've given? It really shouldn't... will test it when I've got access to a properly multi-core machine (as my netbook doesn't show the problem even with the first code.) – Mythologize

@JonSkeet ConncurrentMap returns null if key is absent in map. docs.oracle.com/javase/6/docs/api/java/util/concurrent/… – Vanderhoek

Answer 2

You can try to make all accesses to concurrentHashMap synchronous. So when you get value from map you synchronize on concurrentHashMap and when you remove it. Something like this:

public void doSomething(int i) {
    synchronized (getLockForId(i)) {
        // do stuff
        synchronized (concurrentHashMap) {
            concurrentHashMap.remove(i);
        }
    }
}


public Integer getLockForId(int id) {
    synchronized (concurrentHashMap) {
        concurrentHashMap.putIfAbsent(id, id);
        return concurrentHashMap.get(id);
    }
}

Vanderhoek

Then there remains no point of having this [solution in question] solution :) Because you no more have multiple locks you are only synchronizing on one lock. – Trolley

I think you need both lock. Because when you manipulate with getting/releasing id from map - you use lock on this map. When you some stuff with current id - you have lock on this id. So there are no 2 concurrent threads doing stuff on same id while map is not locked and other threads can work on other ids. – Vanderhoek

Answer 3

Consider using google's LoadingCache instead. It uses weak references, so garbage collection is left to the JVM; you don't have to delete references you no longer need. And it handles the concurrency issues involved with creating a new entry. The code for a value synchronizer would look like this:

import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class ValueSynchronizer<T> {
  private final LoadingCache<T, Lock> valueLocks;

  public ValueSynchronizer() {
    valueLocks = CacheBuilder.newBuilder().build(
      new CacheLoader<T, Lock>() {
        public Lock load(final T id) {
          return new ReentrantLock();
        }
      });
  }

  public void sync(final T onValue, final Runnable toDo)
  {
    final Lock lock = valueLocks.getUnchecked(onValue);
    lock.lock();

    try {
      toDo.run();
    }
    finally {
      lock.unlock();
    }
  }
}

Actually syncing on a value would look like this:

private final ValueSynchronizer<Long> retrySynchronizer 
    = new ValueSynchronizer<>();

@Override
public void onApplicationEvent(final EventThatCanBeSentMultipleTimes event)
{
  retrySynchronizer.sync(event.getEventId(), () ->
  {
    //...Your code here.
  });
}

(licensed under Apache 2.0 license: http://www.apache.org/licenses/LICENSE-2.0)

Periderm