Let me put the conclusion first - locking on private fields does not work for slightly more complicated multi-threaded program. This is because multi-threading is a global problem. It is impossible to localize synchronization unless you write in a very defensive way (e.g. copy everything on passing to other threads).
Here is the long explanation:
Synchronization includes 3 parts: Atomicity, Visibility and Ordering
Synchronized block is very coarse level of synchronization. It enforces visibility and ordering just as what you expected. But for atomicity, it does not provide much protection. Atomicity requires global knowledge of the program rather than local knowledge. (And that makes multi-threading programming very hard)
Let's say we have a class Account having method deposit and withdraw. They are both synchronized based on a private lock like this:
class Account {
private Object lock = new Object();
void withdraw(int amount) {
synchronized(lock) {
// ...
}
}
void deposit(int amount) {
synchronized(lock) {
// ...
}
}
}
Considering we need to implement a higher-level class which handles transfer, like this:
class AccountManager {
void transfer(Account fromAcc, Account toAcc, int amount) {
if (fromAcc.getBalance() > amount) {
fromAcc.setBalance(fromAcc.getBalance() - amount);
toAcc.setBalance(toAcc.getBalance + amount);
}
}
}
Assuming we have 2 accounts now,
Account john;
Account marry;
If the Account.deposit() and Account.withdraw() are locked with internal lock only. That will cause problem when we have 2 threads working:
// Some thread
void threadA() {
john.withdraw(500);
}
// Another thread
void threadB() {
accountManager.transfer(john, marry, 100);
}
Because it is possible for both threadA and threadB run at the same time. And thread B finishes the conditional check, thread A withdraws, and thread B withdraws again. This means we can withdraw $100 from John even if his account has no enough money. This will break atomicity.
You may propose that: why not adding withdraw() and deposit() to AccountManager then? But under this proposal, we need to create a multi-thread safe Map which maps from different accounts to their locks. We need to delete the lock after execution (otherwise will leak memory). And we also need to ensure no other one accesses the Account.withdraw() directly. This will introduce a lots of subtle bugs.
The correct and most idiomatic way is to expose the lock in the Account. And let the AccountManager to use the lock. But in this case, why not just use the object itself then?
class Account {
synchronized void withdraw(int amount) {
// ...
}
synchronized void deposit(int amount) {
// ...
}
}
class AccountManager {
void transfer(Account fromAcc, Account toAcc, int amount) {
// Ensure locking order to prevent deadlock
Account firstLock = fromAcc.hashCode() < toAcc.hashCode() ? fromAcc : toAcc;
Account secondLock = fromAcc.hashCode() < toAcc.hashCode() ? toAcc : fromAcc;
synchronized(firstLock) {
synchronized(secondLock) {
if (fromAcc.getBalance() > amount) {
fromAcc.setBalance(fromAcc.getBalance() - amount);
toAcc.setBalance(toAcc.getBalance + amount);
}
}
}
}
}
To conclude in simple English, private lock does not work for slightly more complicated multi-threaded program.
(Reposted from https://mcmap.net/q/15669/-in-java-critical-sections-what-should-i-synchronize-on)
