Why is synchronized not working properly?

Question

Here's my code:

private int count = 0;

  public synchronized void increment() {
      count++;
  }

 public void doWork() throws InterruptedException {

    Thread t1 = new Thread(new Runnable() {
        public void run() {
            for (int i = 0; i < 5; i++) {
                increment();
                System.out.println(count+"  "+Thread.currentThread().getName());
            }}});

    Thread t2 = new Thread(new Runnable() {
        public void run() {
            for (int i = 0; i < 5; i++) {
                increment();
                System.out.println(count+"  "+Thread.currentThread().getName());
            }}});

    t1.start();
    t2.start();
}

Here's my output:

2  Thread-1
2  Thread-0
3  Thread-1
5  Thread-1
6  Thread-1
4  Thread-0
8  Thread-0
9  Thread-0
7  Thread-1
10  Thread-0

My understanding is that increment is synchronized. So, it should first increment one number and then release the lock and then give the lock to the thread t1 or t2. So, it should increment one number at a time, right?

But why is my code incrementing two or three numbers at a time? Am I doing something wrong (I'm a newbie)?

Answer 1

While count++; indeed is synchronized System.out.println(count+" "+Thread.currentThread().getName()); is not, but it access to count variable.

Even if you synchronize access, it won't help you because next scenario will be still possible:

• Thread 1 increment
• Thread 2 increment
• Thread 1 print value 2
• Thread 2 print value 2

To fix this issue you need to increment and print in same synchronized section. For example you can put System.out.println(count+" "+Thread.currentThread().getName()); into increment method.

Answer 2

One alternative solution without using synchronized.

Since your use case is simple ( just incrimenting the counter and print the value, AtomicInteger is better choice.

import java.util.concurrent.atomic.AtomicInteger;

public class TestCounter{
    private AtomicInteger count = new AtomicInteger(0);

    public void doWork() throws InterruptedException {

        Thread t1 = new Thread(new Runnable() {
            public void run() {
                for (int i = 0; i < 5; i++) {
                    System.out.println(""+Thread.currentThread().getName()+":"+count.incrementAndGet());
                }}});

        Thread t2 = new Thread(new Runnable() {
            public void run() {
                for (int i = 0; i < 5; i++) {
                    System.out.println(""+Thread.currentThread().getName()+":"+count.incrementAndGet());
                }}});

        t1.start();
        t2.start();
    }

    public static void main(String args[]) throws Exception{
        TestCounter tc = new TestCounter();
        tc.doWork();
    }
}

output:

Thread-0:1
Thread-0:3
Thread-0:4
Thread-0:5
Thread-0:6
Thread-1:2
Thread-1:7
Thread-1:8
Thread-1:9
Thread-1:10

Refer to @fabian answer for why these numbers are nor not printed in sequence.

If you expect the sequence in series of numbers in ascending order from 1-10, Threads are not required.

Answer 3

The increment method can be run on the other thread after the increment method returns, but before count is retrieved for the concatenation

count+"  "+Thread.currentThread().getName()

You could e.g. fix this by modifying and retrieving count in one synchronized block:

public synchronized int incrementAndGet() {
    count++;
    return count; // read access synchronized
}

for (int i = 0; i < 5; i++) {
    System.out.println(incrementAndGet()+"  "+Thread.currentThread().getName());
}

Or use the class in the standard library specifically designed for this purpose:

private final AtomicInteger counter = new AtomicInteger(0);

public void doWork() throws InterruptedException {

    Thread t1 = new Thread(new Runnable() {
        public void run() {
            for (int i = 0; i < 5; i++) {
                System.out.println(counter.incrementAndGet() + "  " + Thread.currentThread().getName());
            }
        }
    });

    Thread t2 = new Thread(new Runnable() {
        public void run() {
            for (int i = 0; i < 5; i++) {
                System.out.println(counter.incrementAndGet() + "  " + Thread.currentThread().getName());
            }
        }
    });

    t1.start();
    t2.start();
}

Of course this does not necessarily lead the numbers 1 to 10 being printed in order, just that no number is retrieved more than once. The following output could happen:

2  Thread-0
3  Thread-0
4  Thread-0
1  Thread-1
5  Thread-0
6  Thread-1
7  Thread-0
8  Thread-1
9  Thread-1
10  Thread-1

Answer 4

What actually happens is that your Threads are snapshotting (maybe a another word is better here) the current value of the variable count and display it. You can think of it like there is a blue bucket with the number zero and both Threads are getting the same bucket in the same color and number. They now work individually on those buckets.

If you want them to work on the same bucket, you have to make them atomic e.g. with AtomicInteger or volatile or any other tool from the java concurrent package.

Answer 5

Solution 1: Given by fabian. To give a single function incrementAndGet().

Solution 2: A synchronized block instead of synchronized method (if possible):

Complete code would be like:

private int count = 0;
private Object dummyObject = new Object();

public void increment() {
    count++;
}

public int getCount() {
    return count;
}

public void doWork() throws InterruptedException {

    Thread t1 = new Thread(new Runnable() {
        public void run() {
            for (int i = 0; i < 5; i++) {
                synchronized (dummyObject) {
                    increment();
                    System.out.println(count + "  " + Thread.currentThread().getName());
                }
            }
        }
    });

    Thread t2 = new Thread(new Runnable() {
        public void run() {
            for (int i = 0; i < 5; i++) {
                synchronized (dummyObject) {
                    increment();
                    System.out.println(count + "  " + Thread.currentThread().getName());
                }
            }
        }
    });

    t1.start();
    t2.start();
}