Is there a race condition in this code?

Question

I need to increase a counter in 5 threads until it reaches 500. Whit 5 threads like this, is working. But i need to know if it have a race condition.

Another problem with this code is that it gives me the prints in the wrong order.

class HiloContador extends Thread {

    static int count = 0;

    @Override
    public void run() {
        for (int i = 0; i < 100; i++) {
            synchronized (new Integer(count)) {
                ++count;
                System.out.println(count);
            }
        }
    }
}

Output

4 6 8 9 10 11 12 5 14 15 3 17 18

Some ideas?

Answer 1

Analysis

Is there a race condition in this code?

Yes, there is a race condition: this line represents incorrect synchronisation:

synchronized (new Integer(count)) {

Here is why.
Let's refer to the documentation:

Every object has an intrinsic lock associated with it. By convention, a thread that needs exclusive and consistent access to an object's fields has to acquire the object's intrinsic lock before accessing them, and then release the intrinsic lock when it's done with them. A thread is said to own the intrinsic lock between the time it has acquired the lock and released the lock. As long as a thread owns an intrinsic lock, no other thread can acquire the same lock. The other thread will block when it attempts to acquire the lock.

— Intrinsic Locks and Synchronization (The Java™ Tutorials > Essential Classes > Concurrency).

In the current case the first sentence is critical. The current implementation uses an intrinsic lock of a new object to synchronise on every loop iteration. This is incorrect.

Solution overview

Alternatives to achieve the atomicity of the increment operation:

1. Using the intrinsic lock (synchronized keyword). See the Intrinsic Locks and Synchronization (The Java™ Tutorials > Essential Classes > Concurrency) article.
2. Using the atomic variables (the java.util.concurrent.atomic package: AtomicBoolean, AtomicInteger, AtomicLong, etc. classes). See the Atomic Variables (The Java™ Tutorials > Essential Classes > Concurrency) article.
3. Using the lock objects (the classes of the java.util.concurrent.locks package). See the Lock Objects (The Java™ Tutorials > Essential Classes > Concurrency) article.

Minimal solution

Let's use the atomic variable (solution #3), i.e. the AtomicInteger class: it has the required functionality built in.

Also, let's not extend the Thread class: let's extract the appropriate Runnable interface implementation instead.

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;

public class Program {
    public static void main(String[] args) throws InterruptedException {
        final CountingRunnable countingRunnable = new CountingRunnable();
        final List<Thread> threads = new ArrayList<>();
        for (int i = 0; i < 5; ++i) {
            final Thread thread = new Thread(countingRunnable);
            threads.add(thread);
            thread.start();
        }

        for (final Thread thread : threads) {
            thread.join();
        }
    }

    private static final class CountingRunnable implements Runnable {
        private final AtomicInteger count = new AtomicInteger(0);

        @Override
        public void run() {
            for (int i = 0; i < 100; i++) {
                System.out.println(count.incrementAndGet());
            }
        }
    }
}

Additional references

1. The entire Lesson: Concurrency (The Java™ Tutorials > Essential Classes).
2. Memory Consistency Errors (The Java™ Tutorials > Essential Classes > Concurrency). To understand the happens-before relationship.

Answer 2

Yes, there is a race condition, because you actually synchronize code on different objects. The synchronized block represent a critical section. For entering in a critical section you must acquire a global lock. You can use an object for lock or an Integer. This is how I would implement:

class HiloContador extends Thread {

    static int count = 0;
    static Object lock = new Object();

    @Override
    public void run() {
        for (int i = 0; i < 100; i++) {
            synchronized (lock) {
                ++count;
                System.out.println(count);
            }
        }
    }
}

Now the println will be in the expected order.