Java synchronization not working

Question

I'm trying to get a small program to demonstrate synchronization but for whatever reason, it's not fulfilling my expectations. The point is to make 1000 threads and have them all add 1 to the static Integer object "sum". The output is supposed to be 1000 but I getting different outputs. It's like the addSum() method isn't synchronized at all. I've tried delaying the println, thinking it was printing sum too quickly but that's not the problem. What am I missing here?

public class sumsync implements Runnable {
public static Integer sum = new Integer(0);
public sumsync(){
}

private synchronized void addSum(int i){
    sum += i;
}

@Override
public void run() {
    addSum(1);
}
}

Main class:

public class sumsyncinit {

private static final int max_threads = 1000;

public static void main(String[] args) {

sumsync task = new sumsync();
Thread thread;

    for(int i=0; i<max_threads;i++){
        thread = new Thread(task);
        thread.start();
    }
    System.out.println(sumsync.sum);
}

}

Stephan · Accepted Answer · 2013-03-10 14:33:47Z

5

You are not waiting for your threads to finish, thus you have no guarantee that all the increments have been executed. You are basically just guaranteeing that only one thread at a time is in the addSum method. You might want to use Futures to wait for the result.

answered Mar 10, 2013 at 14:33

Stephan

7,38839 silver badges46 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

bsiamionau Over a year ago

From OP: I've tried delaying the println, thinking it was printing sum too quickly but that's not the problem.

bsiamionau Over a year ago

I tried to add Thread.sleep(1000) before sysout and it works. OP was not right. +1

Anubis Over a year ago

@zvzdhk Huh, I thought I tested that. Apparently not, thanks!

Matt Ball Over a year ago

@JHeikes use Thread#join() to wait for threads to finish executing.

Mik378 · Accepted Answer · 2013-03-10 17:38:32Z

0

Use a ThreadPoolExecutor (http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/ThreadPoolExecutor.html) in order to firstly have some pooling, rather than creating a new thread each time, and secondly, to call its awaitTermination method in order to wait for all threads termination before printing the outcome.

Actually, in your case, you haven't set a mechanism that prevent print execution to happen AFTER all threads finish their variable incrementation. Therefore, print outcome may be random. awaitTerminationacts as a join() on all threads and achieve this requirement.

Besides, making the variable volatile would be useless and unsafe in this case.

Indeed, synchronizedkeyword already acts as a memory barrier and atomicity whereas volatile merely ensures memory barrier.

Further, here the rule to keep in mind when one wants to make a variable volatile:

You can use volatile variables instead of locks only under a restricted set of circumstances. Both of the following criteria must be met for volatile variables to provide the desired thread-safety:
Writes to the variable do not depend on its current value.
The variable does not participate in invariants with other variables.

edited Mar 10, 2013 at 17:38

answered Mar 10, 2013 at 14:38

Mik378

22.2k15 gold badges95 silver badges191 bronze badges

2 Comments

Matt Ball Over a year ago

This doesn't explain why the OP's SSCCE does not behave as expected.

Mik378 Over a year ago

@Matt Ball It was implied when I wrote: "in order to wait for all threads termination before printing the outcome." Actually, print execution may happens before all threads finished the variable incrementation. I'm updating it to be more precise. Thanks :)

sgp15 · Accepted Answer · 2013-03-11 03:38:02Z

-2

You can also make sum an AtomicInteger.

Then addSum() will look as follows.

public static AtomicInteger sum = new AtomicInteger(0);

private void addSum(int i){
    sum.addAndGet(i);
}

UPDATE: Solution above was intended to address the race condition only. After Stephen's comment below I am posting complete solution so that main thread waits for other threads to complete before printing final value.

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicInteger;

public class sumsync implements Runnable {
public static AtomicInteger sum = new AtomicInteger(0);
private CountDownLatch latch;
public sumsync(CountDownLatch latch){

  this.latch = latch;
}

private synchronized void addSum(int i){
    sum.addAndGet(i);
}

@Override
public void run() {
    addSum(1);
    latch.countDown();
}
}

import java.util.concurrent.CountDownLatch;

public class sumsyncinit {

private static final int max_threads = 1000;

public static void main(String[] args) {

CountDownLatch latch = new CountDownLatch(max_threads);

sumsync task = new sumsync(latch);
Thread thread;

    for(int i=0; i<max_threads;i++){
        thread = new Thread(task);
        thread.start();
    }
    try {
      latch.await();
    } catch (InterruptedException e) {
      e.printStackTrace();
    }
    System.out.println(sumsync.sum);
}

}

edited Mar 11, 2013 at 3:38

answered Mar 10, 2013 at 14:39

sgp15

1,2808 silver badges13 bronze badges

2 Comments

Stephan Over a year ago

This does not solve the problem the OP is asking for. It only removes the need for synchronized but you are still running into "the sum is not 1000 issues"

sgp15 Over a year ago

@Stephan - Using AtomicInteger addresses the race condition properly here. The 1000 in the end is not printed because main thread doesn't wait for the threads to end. This is a different problem altogether. That can be solved using a CountDownLatch. I'm posting solution for that too.

Collectives™ on Stack Overflow

Java synchronization not working

3 Answers 3

4 Comments

2 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

4 Comments

2 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related