Why is synchronization for count not working?

Question

I want the final count to be 10000 always but even though I have used synchronized here, Im getting different values other than 1000. Java concurrency newbie.

public class test1 {
    static int count = 0;

    public static void main(String[] args) throws InterruptedException {

        int numThreads = 10;

        Thread[] threads = new Thread[numThreads];

        for(int i=0;i<numThreads;i++){
            threads[i] = new Thread(new Runnable() {
                @Override
                public void run() {
                    synchronized (this) {
                        for (int i = 0; i < 1000; i++) {
                            count++;
                        }
                    }
                }
            });
        }

        for(int i=0;i<numThreads;i++){
            threads[i].start();
        }

        for (int i=0;i<numThreads;i++)
            threads[i].join();

        System.out.println(count);
    }

}

`synchronized (this)`. Each `Thread` is synchronised on itself. Try `synchronized (test1.class)`. — Boris the Spider, Feb 03 '22 at 18:30
Does this answer your question? [Synchronized block not working](https://stackoverflow.com/questions/18029698/synchronized-block-not-working) — Tsyvarev, Feb 03 '22 at 23:40

score 1 · Answer 1 · answered Feb 03 '22 at 18:50

Boris told you how to make your program print the right answer, but the reason why it prints the right answer is, your program effectively is single threaded.

If you implemented Boris's suggestion, then your run() method probably looks like this:

public void run() {
    synchronized (test1.class) {
        for (int i = 0; i < 1000; i++) {
            count++;
        }
    }
}

No two threads can ever be synchronized on the same object at the same time, and there's only one test1.class in your program. That's good because there's also only one count. You always want the number of lock objects and their lifetimes to match the number and lifetimes of the data that they are supposed to protect.

The problem is, you have synchronized the entire body of the run() method. That means, no two threads can run() at the same time. The synchronized block ensures that they all will have to execute in sequence—just as if you had simply called them one-by-one instead of running them in separate threads.

This would be better:

public void run() {
    for (int i = 0; i < 1000; i++) {
        synchronized (test1.class) {
            count++;
        }
    }
}

If each thread releases the lock after each increment operation, then that gives other threads a chance to run concurrently.

On the other hand, all that locking and unlocking is expensive. The multi-threaded version almost certainly will take a lot longer to count to 10000 than a single threaded program would do. There's not much you can do about that. Using multiple CPUs to gain speed only works when there's big computations that each CPU can do independently of the others.

szeak · Answer 2 · 2022-02-03T19:24:14.713

For your simple example, you can use AtomicInteger instead of static int and synchronized.

    final AtomicInteger count = new AtomicInteger(0);

And inside Runnable only this one row:

    count.IncrementAndGet();

Using syncronized blocks the whole class to be used by another threads if you have more complex codes with many of functions to use in a multithreaded code environment.

This code does'nt runs faster because of incrementing the same counter 1 by 1 is always a single operation which cannot run more than once at a moment.

So if you want to speed up running near 10x times faster, you should counting each thread it's own counter, than summing the results in the end. You can do this with ThreadPools using executor service and Future tasks wich can return a result for you.

Why is synchronization for count not working?

2 Answers2