Java synchronize a class instance

Question

I can't seem to find any example of what I want to ask:

Let's say I have in class Account

public void transferMoney(Account fromAccount, Account toAccount, DollarAmount amountToTransfer) { 
    if (fromAccount.hasSufficientBalance(amountToTransfer) { 
       fromAccount.debit(amountToTransfer); 
       toAccount.credit(amountToTransfer);
    }
}

this may cause a race condition when used in an overriden run method, and I have this code in Main:

Account acc1 = new Account(..);
Account acc2 = new Account(..);
Thread t1 = new Thread(new Transfer(..from acc1 to acc2..));
Thread t2 = new Thread(new Transfer(..from acc2 to acc1..));
synchronized (acc1) {
    t1.start();
}
synchronized (acc2) {
    t2.start()
}

Synchronizing these two instances of Account acc1 and acc2 does not prevent the Race Condition, but I don't know why or what synchronizing does exactly in this case! I couldn't find any example about this kind of synchronize.

Answer 1

When you say:

synchronized (acc1) {
    t1.start();
}

that causes the current thread to try to acquire the lock on the object acc1, then, once it has that lock, the current thread starts the thread t1. At that point t1's run method gets called in a new thread, and that new thread can do whatever it wants, it's not subject to any locking. You don't lock a thread as such, threads work by acquiring locks that give them access to shared data. When a thread tries to enter a synchronized block that another thread has acquired the lock for, it has to block until it can get the lock.

Synchronizing doesn't do anything unless the different threads are locking on something that's shared. Here one thread locks on one object and the other thread locks on a different object, so there is no effect.

It would seem like you'd want a lock on acc1 and on acc2:

synchronized (acc1) {
    synchronized (acc2) {
        ... now do the transfer
    }
}

This creates problems because you have to get the locks in the same order or risk deadlock. Also it depends on the instances of acc1 and acc2 being shared among the different threads; if different threads instantiate their own copies (for instance with Hibernate each thread would have its own session and would make its own copy of any persistent entities) this won't work.

In real life you wouldn't do this; for things like account transfers you'd probably use database locking or transactions for this, where the database is the shared resource that limits access. Toy examples can cause confusion sometimes because people get distracted picking apart the toy example. A real-life situation where you would see synchronization is on collections (lists, maps, queues, etc.) that need to be accessed by multiple threads.

Answer 2

I do not really understand what do these synchronized locks exactly lock?

They don't lock anything. "Lock" is a very misleading name in this context.

The synchronized keyword does one thing, and one thing only: It prevents two or more threads from synchronizing on the same object at the same time.

See @misberner 's answer for a good solution to your problem.

Answer 3

synchronized only has an effect if a thread tries to enter a synchronized block while another thread currently is in a synchronized block referring to the same object. Here, only your main thread synchronizes on acc1 and acc2, hence no synchronization will actually take place.

A better design for your case would be to aggregate hasSufficientBalance and debit into a single method, which is synchronized (which is equivalent to the whole method body being enclosed in a synchronized(this) block). This synchronized serves to protect the internal state of the class. Therefore, you will also want to mark the credit method as synchronized.

public synchronized boolean debitIfHasSufficientBalance(DollarAmount amount) {
   if (hasSufficientBalance(amount)) {
     debit(amount);
     return true;
   }
   return false;
}

Your transferMoney could then be rewritten as following, without any explicit synchronization:

public void transferMoney(Account fromAccount, Account toAccount, DollarAmount amountToTransfer) {
  if (fromAccount.debitIfHasSufficientBalance(amountToTransfer)) {
    toAccount.credit(amountToTransfer);
  }
}

Answer 4

Well, you probably don't want Account objects to be in charge of transferring money among themselves. Instead I would recommend implementing some sort of controller object, which facilitates the transfers.

To ensure your controller object can only have a single instance, a good way to go about this is to make it an Enum, which brings along the synchronization stuff "for free" as Josh Bloch would say.

http://www.informit.com/articles/article.aspx?p=1216151&seqNum=3

http://www.journaldev.com/1377/java-singleton-design-pattern-best-practices-with-examples

http://javarevisited.blogspot.com/2012/07/why-enum-singleton-are-better-in-java.html

Remember, with an Enum, there is only ever one that exists in your program, so if you call it from two different threads, one will be blocked until the Enum is released by the other caller. This happens automagically with Enum objects.

public Enum AccountController {

    MyBankProgram,
    ;

    /**
     * Transfers DollarAmount from Account a to Account b.
     */
    public void transferFunds(final Account a, final Account b, DollarAmount d) {
        // do stuff here
        a.subtract(d);
        b.add(d);
    }

}

This would be used as:

AccountController.MyBankProgram.transferFunds(accountA, accountB, new DollarAmount("45.00"));

then your Account objects would only need publically exposed methods of:

public void add(final DollarAmount n);
public void subtract(final DollarAmount n);

And I would recommend these go through your controller object and not be called from anywhere else, you don't want random pieces of code adding or removing funds!