Putting the value in ConcurrentHashMap is atomic or not?

Question

I am working on a project in which I am making connections to database. And I need to see how many times an exception is happening if there are any. I am working with Multithreaded code, meaning multiple threads will be making connection to database and inserting into database. So it might be possible that at some point connection get lost so we need to see how many times those exception has occurred.

So I wrote a below code and in the catch block, I am catching exception and making a counter to increased every time if there is any exeption and putting it in ConcurrentHashMap.

class Task implements Runnable {

     public static final AtomicInteger counter_sql_exception = new AtomicInteger(0);
     public static final AtomicInteger counter_exception = new AtomicInteger(0);
     public static ConcurrentHashMap<String, Integer> exceptionMap = new ConcurrentHashMap<String, Integer>();

     @Override
     public void run() {

     try {

         //Make a db connection and then executing the SQL-

         } catch (SQLException e) {
              synchronized(this) {
                   exceptionMap.put(e.getCause().toString(), counter_sql_exception.incrementAndGet());
              }
              LOG.Error("Log Exception")
          } catch (Exception e) {
              synchronized(this) {
                   exceptionMap.put(e.getCause().toString(), counter_exception.incrementAndGet());
              }
              LOG.Error("Log Exception")
        }
      }
  }

My Question is- Today I had a code review and one of my senior team members said, you won't be needing synchronized(this) on the exceptionMap in the catch block. I said yes we will be needing because incrementing the counter is atomic. Putting a new value in the map is atomic. But doing both without synchronization is not atomic . And he said ConurrentHashMap will be doing this for you.

So does I will be needing synchronized(this) block on that exceptionMap or not.? If not then why? And if Yes then what reason should I quote to him.

Answer 1

if you are tying to count the number of times each exception occured, then you need something like this:

private static final ConcurrentMap<String, AtomicInteger> exceptionMap = new ConcurrentHashMap<String, AtomicInteger>();

private static void addException(String cause) {
  AtomicInteger count = exceptionMap.get(cause);
  if(count == null) {
    count = new AtomicInteger();
    AtomicInteger curCount = exception.putIfAbsent(cause, count);
    if(curCount != null) {
      count = curCount;
    }
  }
  count.incrementAndGet();
}

note that having a static map of exceptions is a resource leak unless you periodically clean it out.

As @dnault mentioned, you could also use guava's AtomicLongMap.

UPDATE: some comments on your original:

• you are correct, that you do need another wrapping synchronized block to ensure that the latest value actually makes it into the map. however, as @Perception already pointed out in comments, you are synchronizing on the wrong object instance (since you are updating static maps, you need a static instance, such as Task.class)
• however, you are using a static counter, but a String key which could be different for different exceptions, thus you aren't actually counting each exception cause, but instead sticking random numbers in as various map values
• lastly, as i've shown in my example, you can solve the aforementioned issues and discard the synchronized blocks completely by making appropriate use of the ConcurrentMap.