My java unit test failed if there is a call to wait method inside a synchronized method

Question

I am learning multi-threads programming in java recently. And I don't understand why the following test case will fail. Any explanation will be much appreciated.

Here is MyCounter.java.

public class MyCounter {
    private int count;

    public synchronized void incrementSynchronized() throws InterruptedException {
        int temp = count;
        wait(100);  // <-----
        count = temp + 1;
    }

    public int getCount() {
        return count;
    }
}

This is my unit test class.

public class MyCounterTest {
    @Test
    public void testSummationWithConcurrency() throws InterruptedException {
        int numberOfThreads = 100;
        ExecutorService service = Executors.newFixedThreadPool(10);
        CountDownLatch latch = new CountDownLatch(numberOfThreads);
        MyCounter counter = new MyCounter();
        for (int i = 0; i < numberOfThreads; i++) {
            service.submit(() -> {
                try {
                    counter.incrementSynchronized();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
                latch.countDown();
            });
        }
        latch.await();
        assertEquals(numberOfThreads, counter.getCount());
    }
}

But if I remove wait(100) from the synchronized method incrementSynchronized, the test will succeed. I don't understand why wait(100) will affect the result.

Answer 1

Solomons suggestion to use sleep is a good one. If you use sleep instead of wait, you should see the test pass.

Using wait causes the thread to relinquish the lock, allowing other threads to proceed and overwrite the value in count. When the thread's wait times out, it acquires the lock again, then writes a value to count that may be stale by now.

The typical usage of wait is when your thread can't do anything useful until some condition is met. Some other thread eventually satisfies that condition and a notification gets sent that will inform the thread it can resume work. In the meantime, since there is nothing useful the thread can do, it releases the lock it is holding (because other threads need the lock in order to make progress meeting the condition that the thread is waiting for) and goes dormant.

Sleep doesn't release the lock so there won't be interference from other threads. For either the sleeping case or the case where you delete the wait call, the lock is held for the duration of the operation, nothing else can change count, so it is threadsafe.

Be aware that in real life, outside of learning exercises, sleeping with a lock held is usually not a great idea. You want to minimize the time that a task holds a lock so you can get more throughput. Threads denying each other the use of a lock is not helpful.

Also be aware that getCount needs to be synchronized as well, since it is reading a value written by another thread.