[FIXED] CoreJava 11th Ed Threading question on client-side locking (synchronized block)

Issue

I am reading the following part on why client-side locking is not recommended as follows:

"Sometimes, programmers use the lock of an object to implement additional
atomic operations—a practice known as client-side locking. Consider, for
example, the Vector class, which is a list whose methods are synchronized.
Now suppose we stored our bank balances in a Vector. Here is
a naive implementation of a transfer method:

public void transfer(Vector accounts, int from, int to, int amount) //
{
accounts.set(from, accounts.get(from) - amount);
accounts.set(to, accounts.get(to) + amount);
System.out.println(. . .);
}
The get and set methods of the Vector class are synchronized, but that
doesn’t help us. It is entirely possible for a thread to be preempted in the
transfer method after the first call to get has been completed. Another
thread may then store a different value into the same position. However, we
can hijack the lock:

public void transfer(Vector accounts, int from, int to, int amount)
{
synchronized (accounts)
{
accounts.set(from, accounts.get(from) - amount);
accounts.set(to, accounts.get(to) + amount);
}
System.out.println(. . .);
}

This approach works, but it is entirely dependent on the fact that the Vector
class uses the intrinsic lock for all of its mutator methods. However, is this
really a fact? The documentation of the Vector class makes no such
promise. You have to carefully study the source code and hope that future
versions do not introduce unsynchronized mutators. As you can see, client-
side locking is very fragile and not generally recommended."

Question:
Since the synchronized (accounts) in the transfer method has obtained the accounts intrinsic lock, why does that dependent on the vector class uses the intrinsic lock for all its mutator method (as highlighted in bold and italic?

Solution

If the only mutation of the accounts Vector was happening in the transfer method, then it wouldn't matter that Vector synchronises its mutators.

But by locking on the same object as the existing mutator methods (i.e. the Vector), we protect against any other mutating operation on the Vector.

It isn't just a transfer done by another thread which could corrupt our data, but, for instance, a deposit performed on the to account after we've read its balance and before we've set it.

And as Holger points out, once you have mutation in one thread and reads in another, even read operations need to be synchronised if you want a consistent view of your data.

As Core Java is suggesting, it's better to encapsulate the data you want to protect, e.g. (toy example)

public class Accounts {
    private final List<Integer> accounts = new ArrayList();

    public synchronized void transfer(int from, int to, int amount) {
        accounts.set(from, accounts.get(from) - amount);
        accounts.set(to, accounts.get(to) + amount);
    }
    
    public synchronized void deposit(int to, int amount) {
        accounts.set(to, accounts.get(to) + amount);
    }
    
    public synchronized List<Integer> getAccountsSnapshot() {
        // don't return our internal data structure, make a defensive copy
        return new ArrayList(accounts);
    }
}

Returning a copy of our data performs two functions:

• We don't provide a reference to our internal data, so a client can't modify the values in the ArrayList directly, without using the API we've provided.
• The client gets a consistent snapshot of the account balances, so they could sum across all the accounts and get a total which was valid at the time they called getAccountsSnapshot. Otherwise, modifications while they are taking the sum might mean they got a total which had never occurred in 'real life'.

Answered By - tgdavies
Answer Checked By - David Marino (JavaFixing Volunteer)