Understanding Memory Visibility in Java Multi-threading

Question

I was recently trying to wrap my head around some Java multi-threading concepts and was writing a small piece of code to help me understand memory visibility and getting synchronization as correct as possible. Based on what I've read, it seems that the smaller the amount of code we hold a lock around, the more efficient our program will be (in general). I've written a small class to help me understand some of the synchronization issues I might run into:

public class BankAccount {
    private int balance_;

    public BankAccount(int initialBalance) {
        if (initialBalance < 300) {
            throw new IllegalArgumentException("Balance needs to be at least 300");
        }
        balance_ = initialBalance;
    }

    public void deposit(int amount) {
        if (amount <= 0) {
            throw new IllegalArgumentException("Deposit has to be positive");
        }
        // should be atomic assignment
        // copy should also be non-shared as it's on each thread's stack
        int copy = balance_;

        // do the work on the thread-local copy of the balance. This work should
        // not be visible to other threads till below synchronization
        copy += amount;

        synchronized(this) {
            balance_ = copy; // make the new balance visible to other threads
        }
    }

    public void withdraw(int amount) {
        // should be atomic assignment
        // copy should also be non-shared as it's on each thread's stack
        int copy = balance_;

        if (amount > copy) {
            throw new IllegalArgumentException("Withdrawal has to be <= current balance");
        }

        copy -= amount;

        synchronized (this) {
            balance_ = copy; // update the balance and make it visible to other threads.
        }
    }

    public synchronized getBalance() {
        return balance_;
    }
}

Please ignore the fact that balance_ should be a double instead of an integer. I know that primitive type reads/assignments are atomic with the exception of doubles and longs so I opted for ints for simplicity

I've tried to write comments inside of the functions to describe my thinking. This class was written to get correct synchronization as well as minimize the amount of code that's under locking. Here's my questions:

1. Is this code correct? Will it ever run into any data / race conditions? Are all of the updates visible to other threads?
2. Is this code just as efficient as just putting a method-level synchronization? I can imagine that as the amount of work we do increases (here, it's just one addition/subtraction), it can cause significant performance issues doe method-level synchronization.
3. Can this code be made more efficient?

Answer 1

Any code that is not inside the synchronized block can be concurrenlty executed by multiple threads, your solution is creating the new balance outside of a synchronized block so it won't work properly. Let's see an example:

int copy = balance_; // 1

copy += amount; //2

synchronized(this) {
   balance_ = copy; // 3
}

1. When the program starts we have _balance = 10
2. Then we start 2 threads that are trying to add 10 and 15 to the balance
3. Thread 1 assigns 10 to the variable copy
4. Thread 2 assigns 10 to the variable copy
5. Thread 2 adds 15 to copy and assign the result to _balance -> 25
6. Thread 1 adds 10 to copy and assign the result to _balance ->20

At the end the BankAccount has 20 but it should be 35

This is the correct way to make it:

public class BankAccount {
    private int balance_;

    public BankAccount(int initialBalance) {
        if (initialBalance < 300) {
            throw new IllegalArgumentException("Balance needs to be at least 300");
        }
        balance_ = initialBalance;
    }

    public void deposit(int amount) {
        if (amount <= 0) {
            throw new IllegalArgumentException("Deposit has to be positive");
        }

        synchronized(this) {
            balance_ += amount;
        }
    }

    public void withdraw(int amount) {
        synchronized (this) {
            if (amount > balance_) {
                throw new IllegalArgumentException("Withdrawal has to be <= current balance");
            }

            balance_ -= amount;
        }
    }

    public synchronized int getBalance() {
        return balance_;
    }
}

Answer 2

This code is prone to race conditions.

Consider this part:

int copy = balance_;
copy += amount;
// here!
synchronized(this) {
    balance_ = copy; // make the new balance visible to other threads
}

What happens if someone calls withdraw or deposit during the "here" section? That second method will change _balance, but that change won't be reflected in your local copy. When you then write copy to the shared variable, it will simply overwrite the value.

The way to handle this would be to do the whole operation — the read, modification, and write — under the exclusive lock. Alternatively, you could use an AtomicInteger, which provides an atomic incrementAndGet method. This can often compile down to hardware primitives called "compare and swap" and is thus very efficient. The disadvantage is that it only provides atomicity for that one operation; if you need some other operation to also be atomic (maybe you also want to increment a depositCounts field?), then AtomicInteger won't work.