Java synchronization not working as (I) expected

Question

Hi, programmer mates. I was testing java threading capabilities with a very simple code (or at least it seemed simple). I have this class Account:

public class Account {
 protected double balance;

 public synchronized void withdraw(double value) {
  this.balance = this.balance - value;
 }

 public synchronized void deposit(double value) {
  this.balance = this.balance + value;
 }

 public synchronized double getBalance() {
  return this.balance;
 }
}

And I have two threads: Depositer, that deposits $10 a thousand times:

public class Depositer extends Thread {
 protected Account account;

 public Depositer(Account a) {
  account = a;
 }

 @Override
 public void run() {
  for(int i = 0; i < 1000; i++) {
   this.account.deposit(10);
  }
 }
}

And Withdrawer, that withdraws $10 a thousand times:

public class Withdrawer extends Thread {
 protected Account account;

 public Withdrawer(Account a) {
  account = a;
 }

 @Override
 public void run() {
  for(int i = 0; i < 1000; i++) {
   this.account.withdraw(10);
  }
 }
}

This arrangement is executed by:

public class Main {
 public static void main(String[] args) {
  Account account = new Account();
  Thread c1 = new Depositer(account);
  Thread c2 = new Withdrawer(account);

  c2.start();
  c1.start();

  System.out.println(account.getBalance());
 }
}

As the methods are sychronized, I just expected that the balance was always 0 at the end, but this not happens sometimes. And I sincerely cannot figure out why. Can someone see where is my fault?

Answer 1

The c2.start() and c1.start() runs the process asynchronously. Your main thread needs to wait for those two threads to finish before printing out the results.

Call

try {
    c2.join();
    c1.join();
} catch (InterruptedException e) {
    e.printStackTrace();
}

before calling println.

See join.

Waits for this thread to die.

Answer 2

You have to wait for both threads to complete, before you inspect the "final balance".

c1.join();
c2.join();
System.out.println(account.getBalance());

Answer 3

You do the following:

c2.start();  // Start a thread in the background
c1.start();  // Start a 2nd thread in the background

// print out the balance while both threads are still running
System.out.println(account.getBalance());

You need to wait for these threads to complete their processing:

c2.start();  // Start a thread in the background
c1.start();  // Start a 2nd thread in the background

try {
    c2.join();  // Wait until the c2 thread completes
    c1.join();  // Wait until the c1 thread completes
} catch (InterruptedException e) {
    // LOG AN ERROR HERE
}

// print out the final balance
System.out.println(account.getBalance());

If you interrupt your main Thread then you'll need to do something with an interrupted exception. Assuming that none of your code does this, you should always, at a minimum, log the Exception. NOTE: You'll get the InterruptedException not if someone interrupts c1 or c2, but if someone interrupts your main thread, the thread that calls join(). If someone has called interrupt() on your main thread but you don't check for it, then you'll probably get the InterruptedException the moment you call join().

Answer 4

In multithreaded code, the definition of "end" is tricky.

The synchronization only means that each operation on the account is mutually exclusive. That is, the account is not being updated at the time you deposit, withdraw, or check the balance.

However, your printout of the balance happens from within the main program, right after the threads are spawned. Since they are independent threads, there are no guarantees that they finish running before you get to the print. Hence, you could be printing before you are done counting to a 1000. Or to put it differently, the sum is 0 at the end, but you are not printing at the end...

You may want to join the threads from main.

Answer 5

This type of concurrency bug is called a race condition. You need to synchronize the threads. By using the join methods as explained by the others you will effectively implement a concurrent Barrier.

Answer 6

You will notice that withdraw is the same as adding a negative amount, so you don't really need it.

amount should be private, not protected as all the operations you can perform on it should be defined in Account and synchronized.