CODEWITHSUNDEEP
javaconcurrencylocking
Developer87
Developer87

Reputation: 2708

Necessity of the locks while working with concurrent hash map

Here is the code in one of my classes: 

 class SomeClass {

   private Map<Integer, Integer> map = new ConcurrentHashMap<>();
   private volatile int counter = 0;
   final AtomicInteger sum = new AtomicInteger(0); // will be used in other classes/threads too
   private ReentrantLock l = new ReentrantLock();

   public void put(String some) {
    l.lock();
    try {
        int tmp = Integer.parseInt(some);
        map.put(counter++, tmp);
        sum.getAndAdd(tmp);
    } finally {
        l.unlock();
    }
   }

   public Double get() {
    l.lock();
    try {
        //... perform some map resizing operation ...
        // some calculations including sum field ...
    } finally {
        l.unlock();
    }
   }

}

You can assume that this class will be used in concurrent environment.

The question is: how do you think is there a necessity of the locks? How does this code smell? :)

Upvotes: 0

Views: 87

Answers (2)

Andy Turner
Andy Turner

Reputation: 140484

Since you always increment counter when you use it as a key to put into this map:

map.put(counter++, tmp);

when you come to read it again:

return sum / map.get(counter);

map.get(counter) will be null, so this results in a NPE (unless you put more than 2^32 things into the map, ofc). (I'm assuming you mean sum.get(), otherwise it won't compile).

As such, you can have equivalent functionality without any locks:

class SomeClass {
  public void put(String some) { /* do nothing */ }
  public Double get() {
    throw new NullPointerException();
  }
}

You've not really fixed the problem with your edit. divisor will still be null, so the equivalent functionality without locks would be:

class SomeClass {
  private final AtomicInteger sum = new AtomicInteger(0);

  public void put(String some) {
    sum.getAndAdd(Integer.parseInt(some));
  }

  public Double get() {
    return sum.get();
  }
}

Upvotes: 0

samzz
samzz

Reputation: 117

Let's look at the operations inside public void put(String some).

  1. map.put(counter++, tmp);
  2. sum.getAndAdd(tmp);

Now let's look at the individual parts.

  1. counter is a volatile variable. So it only provides memory visibility but not atomicity. Since counter++ is a compound operation, you need a lock to achieve atomicity.

  2. map.put(key, value) is atomic since it is a ConcurrentHashMap.

  3. sum.getAndAdd(tmp) is atomic since it is a AtomicInteger.

As you can see, except counter++ every other operation is atomic. However, you are trying to achieve some function by combining all these operations. To achieve atomicity at the functionality level, you need a lock. This will help you to avoid surprising side effects when the threads interleave between the individual atomic operations.

So you need a lock because counter++ is not atomic and you want to combine a few atomic operations to achieve some functionality (assuming you want this to be atomic).

Upvotes: 1

Related Questions