Styjun

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? :)

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).

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();