Nested synchronized block

Question

Let's imagine I have next classes:

public class Service {
    public void transferMoney(Account fromAcct, Account toAcct, int amount) {
      synchronized (fromAcct) {
        synchronized (toAccount) { // could we use here only one synchronized block?
            fromAcct.credit(amount);
            toAccount.debit(amount);
        }
      }
    }
}

class Account {
  private int amount = 0;

  public void credit(int sum) {
    amount = amount + sum;
  }

  public void debit(int sum) {
    amount = amount - sum;
  }
}

For example I know that we could change state of fromAcct and toAcct objects only in transferMoney method. So could we rewrite our method with one synchronized block?

public class Service {
 private final Object mux = new Object();

 public void transferMoney(Account fromAcct, Account toAcct, int amount) {
      synchronized(mux) {
        fromAcct.credit(amount);
        toAcct.debit(amount);
      }
 }
}

Talishatalisman

A) No. B) That is not safe. https://mcmap.net/q/1776542/-how-to-ensure-locking-order-to-avoid-deadlock/34397 – Mariahmariam

What if you had two Service instances? – Monaural

If you are trying to make the 2 operations (credit + debit) transactional, synchronizing is not the way to do it. If your goal is to make sure you don't corrupt an account's balance if multiple threads move money around in the same account at the same time, then your synchronizing should occur from within the Account class. – Lederhosen

@Mariahmariam could you please explain your A) answer? I agree that we could have dedlock here. – Talishatalisman

@immibis let's imagine that we have only one Service instance. – Talishatalisman

@SamStanojevic I thought that synchronized makes operation transactional... By two synchronized block from first example we guarantee that we don't corrupt our account balance during multiple threads execution. Am I right? – Talishatalisman

@lurii: No. What do you think happens if an exception occurs during the debit step? Then you end up with a credit without a matching debit, and the synchronization didn't help you one bit to protect you against that. What you need is a database to manage a transaction for you. – Lederhosen

well ... let's not make this a transaction problem. it's a toy app for education purpose towards java concurrency. – Rhines

@bayou.io: Couldn't agree with you more. Unfortunately, I think OP is confusing thread safety with transactions. – Lederhosen

Answer 1

Unless you have a very unusual and particular need that I can't understand, it seems to me that your goal should be to protect the account balance from being corrupted by multiple threads attempting to credit or debit an account at the same time.

The way to do that would be like this:

public class Service {
    public void transferMoney(Account fromAcct, Account toAcct, int amount) {
        fromAcct.credit(amount);
        toAccount.debit(amount);
    }
}

class Account {
    private final object syncObject = new Object();
    private int amount = 0;

    public void credit(int sum) {
        synchronized(syncObject) {
            amount = amount + sum;
        }
    }

    public void debit(int sum) {
        synchronized(syncObject) {
            amount = amount - sum;
        }
    }
}

If your goal during the money transfer is to always ensure that both the credit and debit actions occur as one transaction, or atomically, then using synchronizing is not the right approach. Even in a synchronized block, if an exception were to occur, then you lose the guarantee that both actions will occur atomically.

Implementing transactions yourself is a very complex topic, which is why we normally use databases to do that for us.

EDIT: OP asked: What is the difference between my example (one synchronized block mux) and yours synchronized in Account class?

That's a fair question. There are a few differences. But I would say that the main difference is that, ironically, your example over-synchronizes. In other words, even though you are now using a single synchronized block, your performance is actually going to be much worse, potentially.

Consider the following example: You have 4 different bank accounts: Let's name them A, B, C, D. And now you have 2 money transfers that are initiated at the exact same time:

1. Money transfer from account A to account B.
2. Money transfer from account C to account D.

I think that you would agree that because the 2 money transfers are occurring on completely separate accounts, that there should be no harm (no risk of corruption) in executing both money transfers at the same time, right?

Yet, with your example, money transfers can only execute one after another. With mine, both money transfers occur simultaneously, yet safely as well. I will only block if both money transfers attempt to "touch" the same accounts.

Now imagine if you use this code to process hundreds, thousands or more concurrent money transfers. Then there is no doubt that my example will perform A LOT better than yours, while still retaining thread safety, and protecting the correctness of an account's balance.

In effect, my version of the code conceptually behaves a lot more like your original 2-synchronized block code. Except for the following improvements:

• Fixes the potential deadlock scenario.
• Intent is clearer.
• Provides better encapsulation. (Meaning that even if some other code outside the transferMoney method were to try to debit or credit some amounts, I would still retain thread safety, while you would not. I know you said that this is not your case, but with my version, the design absolutely guarantees it)

Lederhosen

credit before debit? :) It's OP's fault though to confuse credit and debit. – Rhines

@SamStanojevic thanks for reply! Sorry I wasn't clear with you. Yes, my goal was to protect the account balance from being corrupted by multiple threads executing. So for this purposes as you mentioned early I should use synchronized in Account methods. But if I use synchronized in transferMoney method then it should protect my code from multiple thread executing? – Talishatalisman

@lurii: yes, if you use synchronized in transferMoney method then it will protect your code from multiple threads executing in that block of code at the same time. But it won't give you any transactional guarantees. It's very important that you understand the fundamental difference between thread safety and transactions. They are 2 completely different things, that are also accomplished very differently. – Lederhosen

@SamStanojevic thanks for clarify. I'll apply your answer, just one question. What is the difference between my example (one synchronized block mux) and yours synchronized in Account class? Sorry that asking basic things, but it's really important for me to know this. – Talishatalisman

@lurii: I edited my post to answer your follow-up question. I couldn't give you a proper response in the comments. – Lederhosen

Answer 2

It looks like you want to implement Transaction by synchronization. There is nothing common between them. Transaction provides integrity of the operation - do all actions or roll them all back. Synchronization assures that data is changed from only one thread at a time. For example, transaction assures that if you take money from one account and not put them to another then first action is undo - money are not withdrawn. Synchronization checks that if two different dudes put 2 pennies to the bank in exact same moment then bank will have 4 pennies and not only 2 because your program adds money to the same account based on previous value.

Coelom