Why the result which the system should print to me changes every time(java concurrency)?

Question

I have a problem in concurrency programming in java. Please look at the code below. The result which system should print to me, changes every time I run the program. Although I’ve synchronized the operation of adding value to sub variable, but the result changes every time. I think I’ve made a mistake somewhere. But I do not know where.

public class Test {

    public static void main(String[] args) {

        final MyClass mClass = new MyClass();
        int size = 10;
        final CountDownLatch cdl = new CountDownLatch(size);
        for(int i = 0; i < size; i++){
            Thread t = new Thread(new Runnable() {
                @Override
                public void run() {
                    for(int number = 0; number < 100000; number++){
                        mClass.addToSub(number);
                    }
                    cdl.countDown();
                }
            });
            t.start();
        }
        try {
            cdl.await();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        //the result changes every time!!!!!!!!
        System.out.println(mClass.getSub());
    }

    public static class MyClass {
        private Long sub = 0L;
        public long getSub() {
            synchronized (sub) {
                return sub;
            }
        }
        public void addToSub(long value){
            synchronized (sub) {
                sub += value;
            }
        }
    }

}

A similar one https://stackoverflow.com/questions/10324272/why-is-it-not-a-good-practice-to-synchronize-on-boolean — xingbin, Apr 10 '18 at 13:43

Thomas Timbul · Answer 1 · 2018-04-10T14:42:56.760

In addToSub you are changing the value on which you synchronize. Effectively this means that there is no synchronization at all.

Either sync on this, or even better, use AtomicLong and avoid both your problem as well as synchronization overhead (Thread contention):

public static class MyClass {
    private AtomicLong sub = new AtomicLong();
    public long getSub() {
        return sub.get();
    }
    public void addToSub(long value){
        sub.addAndGet(value);
    }
}

The Atomic* classes are specifically designed for this type of usecase, where a single variable is updated by multiple Threads, and where synchronize could result in heavy thread contention. If you are dealing with Collections, look towards the ones in java.util.concurrent.*

Edit: Thanks for the correction of addAndGet vs incrementAndGet.

`incrementAndGet` -> `addAndGet`? – OldCurmudgeon Apr 10 '18 at 13:54 — OldCurmudgeon, Apr 10 '18 at 13:54

Julius Häger · Accepted Answer · 2018-04-16T12:53:40.680

What you are getting wrong here is not the multi-threading. What is causing this issue is a java feature called auto-boxing.

Your variable sub has the type Long which is a reference to an object (Long and long are different).

You need to have an object to synchronize on in java so you can not use just a normal long.

The problem here is that a Long is immutable meaning the value does not change. So when you do sub += value you are actually doing sub = Long.valueOf(sub.longValue() + value) witch is creating a new object.

So the current thread only has the previous object locked so new threads can still change the reference sub.

What you want to do is synchronize on a reference that wont change, i.e this

public void addToSub(long value){
    synchronized (this) {
        sub += value;
    }
}

Or more terse:

public synchronized void addToSub(long value) {
    sub += value;
}

And you should probably use long and not Long.

EDIT

As noted in Thomas Timbuls answer you probably want to use AtomicLong as that gives you thread-safety by default and potentially much better performance (as the threads don't need to wait for each-other).

score 1 · Answer 3 · answered Apr 10 '18 at 13:34

You're synchronizing on a non-final value:

synchronized (sub) {

This means that as soon as you change it to some other value:

sub += value;

anything which isn't already waiting at the synchronized block can proceed, because nothing is holding the monitor for this new value.

Synchronize on this instead (or some other unchanging value):

synchronized (this) {

score 1 · Answer 4 · answered Apr 10 '18 at 13:40

sub is an object (Long), change to long and add a private Object for the synchronized. Then it will work.

public static class MyClass {
        private Object locker = new Object();
        private long sub = 0L;
        public long getSub() {
            synchronized (locker) {
                return sub;
            }
        }
        public void addToSub(long value){
            synchronized (locker) {
                sub += value;
            }
        }
}

Why the result which the system should print to me changes every time(java concurrency)?

4 Answers4