JUnit assertion for concurrency test I'm learning Java concurrency by a TDD approach. Therefore I wrote a basic test case in JUnit to test the behaviour / expected results of 2 threads. I'm looking for feedback for this code. In particular, the feedback for using CountDownLatch. I'm not sure if it is used correctly.

package io.mincongh.concurrency;

import static org.junit.Assert.assertEquals;

import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

/**
 * @author Mincong Huang
 */
public class BasicConcurrencyTest {

  private CountDownLatch countDownLatch;
  private Map&lt;Integer, Integer&gt; map;

  @Before
  public void setUp() {
    countDownLatch = new CountDownLatch(2);
    map = new ConcurrentHashMap&lt;&gt;();
  }

  @After
  public void tearDown() {
    countDownLatch = null;
    map = null;
  }

  private class CounterThread extends Thread {

    @Override
    public void run() {
      for (int i = 0; i &lt; 10; i++) {
        int prev = map.getOrDefault(i, 0);
        map.put(i, prev + 1);
        try {
          Thread.sleep(100);
        } catch (InterruptedException e) {
          e.printStackTrace();
        }
      }
      countDownLatch.countDown();
    }
  }

  @Test
  public void testCounterThread() throws InterruptedException {
    CounterThread t1 = new CounterThread();
    CounterThread t2 = new CounterThread();
    t1.start();
    t2.start();
    int tries = 0;
    while (countDownLatch.getCount() &gt; 0 &amp;&amp; tries &lt; 3) {
      countDownLatch.await(1, TimeUnit.SECONDS);
      tries++;
    }
    assertEquals(2, (int) map.get(1));
  }

}
Monitor

I encourage you to think about critical code fragments and restrict concurrent access to it.

The critical code fragments are the following statements:

...
int prev = map.getOrDefault(i, 0);
map.put(i, prev + 1);
...


They have to be executed single-threaded. As you allow multithreaded execution here, two threads may execute "getOrDefault" directly in sequence thanks to ConcurrentHashMap. But they both will get 0! So your next put-statement will bring your model into an inconsistent state.

So the first thing to do is to define a correct Monitor:

public class CountAtIndex {

    private HashMap&lt;Integer, Integer&gt; map;

    public CountAtIndex() {
        map = new HashMap&lt;&gt;();
    }

    public synchronized void countAt(int index) {
        int previousValue = map.getOrDefault(index, 0);
        map.put(index, previousValue + 1);
    }

    public synchronized int get(int index) {
        return this.map.get(index);
    }

}


I do not use a ConcurrentHashMap as it is not necessary anymore. And it is wrong considering the usecase. Those datastructures are not that beneficial as they seem in the first place as you have to know the usecase to chose the proper datastructure to be synchronized.

Synchronizing is done via the synchronized method modifier. Both, the read access method and the modification access method allow only ONE thread to be in the monitor. 

Some may argue to avoid the synchronized modifier because it will introduce a lock on a visible object. Other developers may use the object as a lock as well and a deadlock may happen. My point of view is: If you put a lock on an object that you do not "own" you should know what you are doing. And I think it is a wrong message to allow other developers to not know what they are doing.

Thread

The next to adapt is your thread. First you should define a Runnable instead of deriving from Thread directly. Furthermore the thread should know the usecase specific thread-safe datastructure:

private class CounterRunnable implements Runnable {

    private CountAtIndex monitor;

    public CounterRunnable(CountAtIndex monitor) {
        this.monitor = monitor;
    }

    @Override
    public void run() {

        for (int index = 0; index &lt; 10; index++) {

            try {
                monitor.countAt(index);
                Thread.sleep(100);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }

        }

    }

}


Then you can use the Runnable to create a Thread.

...
Thread counterThread = new Thread(new CounterRunnable(countAtIndex))
...


Test

The only thing to determine thread-safety is to acknowledge that your monitor works. All other things can be tested without considering concurrency.

To determine if a monitor is working is to "attack" the critical code fragments.

Write access

As mentioned above following statements are critical:

...
int prev = map.getOrDefault(i, 0); // (1)
map.put(i, prev + 1); // (2)
...


A correct test would be to ...

... run a thread executing statement 1 and stop

... run a second thread and check if it runs into the monitor lock

... if the second thread ran into the monitor lock -> continue with thread 1

... If the thread 1 leaves the monitor -> thread 2 should enter the monitor, execute the statements and leave the monitor.

Read access

Another issue is that the HashMap is not thread-safe anymore so the "get"-method of "CountAtIndex" must prevent modificiation access during read access.

Reading concurrently only is never a problem. So this is not an issue of concurrency.

To test this scenario you have to ...

... start a thread that is modifying (countAt) and stop before it will leave the monitor.

... start thread 2 that will query the index count (get). You have to check if it ran into the monitor lock.

... thread 1 then has to continue and should leave the thread

... thread 2 now should continue and get the proper value.