3
\$\begingroup\$

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;
  }

  private class CounterThread extends Thread {

    @Override
    public void run() {
      for (int i = 0; i < 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() > 0 && tries < 3) {
      countDownLatch.await(1, TimeUnit.SECONDS);
      tries++;
    }
    assertEquals(2, (int) map.get(1));
  }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Tunaki Feb 12 '17 at 15:42
  • \$\begingroup\$ @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. \$\endgroup\$ – Mincong Huang Feb 12 '17 at 15:56
  • \$\begingroup\$ 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. \$\endgroup\$ – oopexpert Feb 13 '17 at 17:34
1
\$\begingroup\$

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

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

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Mincong Huang Feb 15 '17 at 21:10
  • \$\begingroup\$ 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... \$\endgroup\$ – oopexpert Feb 15 '17 at 22:25
  • \$\begingroup\$ 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/… \$\endgroup\$ – Mincong Huang Feb 15 '17 at 22:37
  • \$\begingroup\$ I can send you my ideas per mail. But it is only raw material I don't want to publish right now. \$\endgroup\$ – oopexpert Feb 15 '17 at 22:47
  • \$\begingroup\$ Yes, I would like to have it. My email is mincong.h@gmail.com . Thank you for spending so much time on this question \$\endgroup\$ – Mincong Huang Feb 15 '17 at 22:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.