Threads changing the same Variable wont synchronize

Question

My problem is that the code should increment a 1000 times and then output it. But sometimes a isn't 1000 at the end.

public class Counter extends Thread {
    private static Integer a = 0;
    
    public void run() {
        for (int i = 0; i < 100; i++) {
            a++;
        }
    }

    public static void main(String[] args) {
        Counter[] ca = new Counter[10];
        for (int i = 0; i < 10; i++) {

            ca[i] = new Counter();
            ca[i].start();
        }

        for (Counter c : ca) {
            try {
                c.join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
        System.out.println(a);
    }

This code is the original code that is obviously not going to work because I have multiple Threads accessing the variable a. I've tried putting a synchronized(this) around a++; and marking a as volatile but I still sometimes get a false Result. The only way I've found to make it work reliably it to put join() into the for loop, but that kind of defeats the point of using Threads in the first place. Any help is appreciated.

`synchronized(this)` wouldn't work since `this` would be the instance of `Counter`, i.e. each thread would only synchronize with itself. `synchronized(Counter.class)` would be better - or use `AtomicInteger`. — Thomas, Jul 12 '21 at 15:35
Those will work but `AtomicInteger` would actually be better in this particular case. — markspace, Jul 12 '21 at 15:36
Here are multiple things at work. `Integer`s are immutable --- the field is static, and you create multiple instances of `Counter`, hence synchronizing on `this` is useless -- I would recommend using an `AtomicInteger`. — Turing85, Jul 12 '21 at 15:36
This question also has about a billion duplicates. Here's one: https://stackoverflow.com/questions/3519664/difference-between-volatile-and-synchronized-in-java — markspace, Jul 12 '21 at 15:37
Also, you of course have to wait for your worker threads to finish if you want to display the correct result in your main thread. Using `Thread#join()` is the standard way to do so. — Janez Kuhar, Jul 12 '21 at 15:42
Thanks everyone. Makes sense that ```synchronized(this)``` can't work. I've also tried ```synchronized(a)```, which doesn't work either, but I'm guessing that's due to the same reason. ```synchronized(Counter.class)``` and ```AtomicInteger``` work. — nordlad, Jul 12 '21 at 15:50
`synchronized(a)` doesn't work because `a` is an Integer, and Integers are immutable. It gets replaced every time you update it. It's an object and you synchronize on the object but the reference to that object just gets replaced by a reference to a new object. (Anytime you use an object like this to synchronize on, it's often a good idea to make that object `final` just to prevent it from accidentally being changed like this. If you did that you'd see your error right away.) — markspace, Jul 12 '21 at 16:09

score 0 · Answer 1 · answered Jul 12 '21 at 16:05

There are several problems in the code you posted, and all them a reported in the comments to your qestion. I try to show the major ones:

Variable a is Integer which is immutable, this means that a++ really means create a new Ineger instance containing the old value plus 1.
the variable a is updated by multiple threads concurrenlty without synchonizazion, this means that the operation a=a+1 canbe split in read a, increment a; if two or more thread read a at the same time the the increment by one the same value

Here is a modified version that uses a Lock to synchronize access to the resource. The main scope of this code is to show that the access to a must be synchronixed between thread; in order to get a "clean design" you must refactor/change a lot of other things.

import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock;

public class SyncProblem { private static int a=0;

private static class IncThread extends Thread {

    private Lock lock;

    public IncThread(Lock lock) {
        this.lock=lock;
    }

    public void run() {
        lock.lock();
        try {
            for (int i = 0; i < 100; i++) {
                a++;
            }
        } finally {
            lock.unlock();
        }
    }
}



public static void main(String[] args) {
    Lock lock= new ReentrantLock();
    IncThread[] ca = new IncThread[10];
    for (int i = 0; i < 10; i++) {

        ca[i] = new IncThread(lock);
        ca[i].start();
    }

    for (IncThread c : ca) {
        try {
            c.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
    System.out.println(a);
}

}

Threads changing the same Variable wont synchronize

1 Answers1