# 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<Integer, Integer> map;

@Before
public void setUp() {
countDownLatch = new CountDownLatch(2);
map = new ConcurrentHashMap<>();
}

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

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

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

}

• Why are you unit testing a class that is only present in the test? Shouldn't CounterThread be main code? In which case it cannot access the countDownLatch anymore. – Tunaki Feb 12 '17 at 15:42
• @Tunaki, I wrote it in such way to make it easier to see the logic, since the core-class and the test-class are in the same location. But you're right, I should separate them. – Mincong Huang Feb 12 '17 at 15:56
• For unit testing concurrency I see the necessity to introduce additional code fragments that would regarded as "boiler-plate" in other cases. The question is: Is the CountDownLatch an appropriate code fragment for it? Of course, the way it is test code and production code currently are coupled does less sense. – oopexpert Feb 13 '17 at 17:34

# Monitor

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<Integer, Integer> map;

public CountAtIndex() {
map = new HashMap<>();
}

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.

private class CounterRunnable implements Runnable {

private CountAtIndex monitor;

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

@Override
public void run() {

for (int index = 0; index < 10; index++) {

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

}

}

}


Then you can use the Runnable to create a Thread.

...
...


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

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.

• How can I produce the your scenarios of "Write Access" and "Read Access" in my tests? For example, I don't know how to stop a running thread just after step 1. – Mincong Huang Feb 15 '17 at 21:10
• I will certainly provide a schema. The point is: You have to control the control flow of each thread involved in this szenario AND introducing artefacts that are within production code and that are useless in production mode. I am curious how much effort you would expect to realize even such a simple szenario. But here we go... – oopexpert Feb 15 '17 at 22:25
• I agree with you: even though my question was in a very simple scenario, it's difficult to assert the synchronized situation. Maybe because we are in a very low level of Java. We can stop here and assume it works. As for the control flow, I can use the IDE to toggle wait-points as described here vmlens.com/articles/… – Mincong Huang Feb 15 '17 at 22:37
• I can send you my ideas per mail. But it is only raw material I don't want to publish right now. – oopexpert Feb 15 '17 at 22:47
• Yes, I would like to have it. My email is mincong.h@gmail.com . Thank you for spending so much time on this question – Mincong Huang Feb 15 '17 at 22:51