Issue in JAVA synchronization with static Object as lock

Question

Please refer the below code where 200 threads are trying to increment the counter variable simultaneously.

The program tries to do the same 100 times. It prints the value of variable in case the incremented value doesn't reach to 200 - meaning some threads interfered with each other.

To avoid interference, I used synchronized keyword. It works well if I use synchronized on static lock object (commented line), but if I use synchronized on counter variable, I still see many times the value doesn't get incremented to 200.

import java.util.ArrayList;
import java.util.List;

public class Class {
    public static Integer counter = new Integer(0);
    
    //public static Object lock = new Object();

    static void increment() {
        synchronized (counter) {
            counter++;
        }
    }

    public static void main(String[] args) throws InterruptedException {
        for (int j = 0; j < 100; j++) {
            counter = 0;
            List<Thread> list = new ArrayList<>();
            for (int i = 0; i < 200; i++) {
                Thread t = new Thread(() -> increment());
                list.add(t);
                t.start();
            }
            for (Thread t : list) {
                t.join();
            }
            if (counter < 200){
                System.out.println(counter);
            }
        }
    }
}

What is the reason for such behaviour?

`counter++` amounts to `counter = Integer.valueOf(counter.intValue()+1);`, which reassigns the `counter` variable, so synchronizing on its old (or new) value is instantly futile. — user207421, Jul 04 '23 at 11:22

spongebob · Accepted Answer · 2023-07-04T13:12:32.510

Integer is immutable, so counter++ resolves to and assigns a different instance for the incremented value.

So synchronized (counter) {} synchronizes on a different monitor when different instances of Integer become visible to the current thread.

The solution is to use AtomicInteger, which features a getAndIncrement() method which increments the value as if a lock was held.

private static final AtomicInteger counter = new AtomicInteger();

static void increment() {
    counter.getAndIncrement();
}

// ...
counter.setPlain(0);
// ...
if (counter.getPlain() < 200) {
// ...

setPlain()/getPlain() can be used instead of set()/get(), since Thread#start() and Thread#join() make sure what happened before is visible and what happens next can see what happened before, respectively.

Note that a method reference can be used here:

Thread t = new Thread(Class::increment);

Note that Integer(int) is deprecated:

Integer counter = 0;

I'm on board with most of this answer, but `setPlain` still seems unsafe to me, since multiple threads are read-update-write-ing the same value. If you put `setPlain` inside a `synchronized` block, sure...that would work I suppose. — Max, Jul 04 '23 at 23:28
@Max: when `setPlain` is used before any of the accessing threads are started, then it's safe, since there's no risk of race conditions. — Joachim Sauer, Jul 05 '23 at 07:49

score -3 · Answer 2 · answered Jul 04 '23 at 10:37

Integer is not a right choice as a lock object. For that purpose AtomicInteger has been developed. So the following code should work as expected.

public class Class {
public static AtomicInteger counter = new AtomicInteger(0);

static void increment() {
    synchronized (counter) {
        counter.incrementAndGet();
    }
}

public static void main(String[] args) throws InterruptedException {
    for (int j = 0; j < 100; j++) {
        counter.set(0);
        List<Thread> list = new ArrayList<>();
        for (int i = 0; i < 200; i++) {
            Thread t = new Thread(Class::increment);
            list.add(t);
            t.start();
        }
        for (Thread t : list) {
            t.join();
        }
        if (counter.get() < 200) {
            System.out.println(counter);
        }
    }
}

}

Also I would recommend to research AtomicInteger type in details to get a real understanding how it works under the hood and why Integer can not be used in a case like yours. https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/atomic/AtomicInteger.html Practical uses for AtomicInteger

1) Thanks but can you explain why Integer is not a right choice as a lock object? End of day, it is also an object so should be okay to use as mutex.. 2) I think, if I use AtomicVariable and something like `incrementAndGet` then I don't even need `synchronized`. It will break only in case increment is done like `counter.set(counter.get()+1)` — Kumar, Jul 04 '23 at 10:47
Does this answer to your question - https://stackoverflow.com/questions/659915/synchronizing-on-an-integer-value ? — Maksym Kosenko, Jul 04 '23 at 10:50
Yes, AtomicInteger is thread safe, in that particular scenario synchronization is unnecessary. — Maksym Kosenko, Jul 04 '23 at 10:59

score -5 · Answer 3 · answered Jul 04 '23 at 10:33

-5

public static volatile Integer counter = new Integer(0);

Use volatile to avoid visibility issues among threads. This should fix the problem.

answered Jul 04 '23 at 10:33

Vidhu Shekhar

1
3

The issue is with locking, I don't think `volatile` will solve it. Anyways tried and didn't work. – Kumar Jul 04 '23 at 10:38
It will not help for sure, more details [here](https://stackoverflow.com/questions/9749746/what-is-the-difference-between-atomic-volatile-synchronized) – Maksym Kosenko Jul 04 '23 at 10:43

Issue in JAVA synchronization with static Object as lock

3 Answers3