Synchronized() block is not working as expected

Question

private static Integer balance=0;

public static void deposit(final int amt) {
    Thread t = new Thread(new Runnable() {
        public void run() {
            synchronized(balance) {
                System.out.println("Balance at start is: "+balance);        
                balance+=amt;
                System.out.println("deposited " + Integer.toString(amt) + " to funds. Now at " + Integer.toString(balance));
            }
        }
    });
}

When I run the above simple deposit function, I expect that two threads should not enter at same time in synchronized block. But with operation Sequence as below:

Depo100
Depo200
Depo700

Output is as below:

------------------
Balance at start is: 0
deposited 100 to funds. Now at 100
Balance at start is: 100
Balance at start is: 100
deposited 700 to funds. Now at 800
deposited 200 to funds. Now at 1000

As we can see two threads entered at same time in the synchronized block and accessed balance object which is NOT expected. What I am doing wrong here ? I am newbie for Multithreading. Thanks in advance.

`balance` is a different object each time. There's no mutual exclusion. — Marko Topolnik, Oct 11 '16 at 08:05
Note that you don't need to call `Integer.toString(amt)` when you are building a string using `+`: string concatenation automatically does that for you. — Andy Turner, Oct 11 '16 at 08:15

score 10 · Accepted Answer · edited May 23 '17 at 12:01

Integer (like all primitive wrapper classes) is immutable. Every time you "add a number to it", you're actually setting the field to a new instance:

balance += amt;

is actually evaluated as follows:

balance = Integer.valueOf(balance.intValue() + amt);

So you're synchronizing on different objects each time (unless amt happens to be zero, and balance.intValue() is in the range cached by your JVM's implementation of Integer.valueOf).

You need a fixed value that you can synchronize on. You can use a fixed reference with a mutable value, for example an Integer[] with length 1, or an AtomicInteger (although synchronizing on an AtomicInteger always feels a bit wrong to me - but actually, you don't need to use synchronization because you can use AtomicInteger.getAndIncrement or AtomicInteger.incrementAndGet).

Note that you should probably make the reference to the thing you synchronize on final, to avoid accidentally setting it to some different value and breaking the mutual exclusion again.

See this question for more details.

Thanks! I will try to get my head around AtomicInteger. Could you explain more on your Note part plz. — crazyThread, Oct 11 '16 at 08:16

score 0 · Answer 2 · edited May 23 '17 at 11:45

As Andy Turner pointed, it is because synchronization is done on an immutable instance which is being modified inside synchronized block.

Solution 1: As suggested by Andy, to use AtomicInteger.

Solution 2: To use synchronization on a dummy object like this:

private static Object dummy = new Object();

private static Integer balance = 0;

public static void deposit(final int amt) {
    Thread t = new Thread(new Runnable() {
        public void run() {
            synchronized (dummy) {
                // System.out.println(balance.hashCode());
                System.out.println("Balance at start is: " + balance);
                balance += amt;
                System.out.println(
                        "deposited " + Integer.toString(amt) + " to funds. Now at " + Integer.toString(balance));
            }
        }
    });
    t.start();
}

Synchronized() block is not working as expected

2 Answers2