Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.

...

While client-side locking is acceptable when the thread-safe class commits to and clearly documents its locking strategy, Goetz and colleagues caution against its misuse [Goetz 2006]:

...

The documentation of a class that supports client-side locking should explicitly state its applicability. For example, the class java.util.concurrent.ConcurrentHashMap<K,V> should not be used for client-side locking because its documentation [API 20062014] states that:

However, even though all operations are thread-safe, retrieval operations do not entail locking, and there is not any support for locking the entire table in a way that prevents all access. This class is fully interoperable with Hashtable in programs that rely on its thread safety but not on its synchronization details.

Use of client-side locking is permitted only when the documentation of the class recommends it. For example, the documentation of the synchronizedList() wrapper method of java.util.Collections class [API 20062014] states:

In order to guarantee serial access, it is critical that all access to the backing list is accomplished through the returned list. It is imperative that the user manually synchronize on the returned list when iterating over it. Failure to follow this advice may result in non-deterministic behavior.

When the backing list is inaccessible to an untrusted client, this advice is consistent with rule LCK04-J. Do not synchronize on a collection view if the backing collection is accessible.

...

This noncompliant code example uses a thread-safe Book class that cannot be refactored. Refactoring might be impossible, for example, when the source code is unavailable for review or when the class is part of a general library that cannot be extended.

Code Block

final class Book {
  // Could change its locking policy in the future
  // to use private final locks
  private final String title;
  private Calendar dateIssued;
  private Calendar dateDue;

  Book(String title) {
    this.title = title;
  }

  public synchronized void issue(int days) {
    dateIssued = Calendar.getInstance();
    dateDue = Calendar.getInstance();
    dateDue.add(dateIssued.DATE, days);
  }

  public synchronized Calendar getDueDate() {
    return dateDue;
  }
}

This class fails to commit to its locking strategy (that is, it reserves the right to change its locking strategy without notice). Furthermore, it fails to document that callers can safely use client-side locking. The BookWrapper client class uses client-side locking in the renew() method by synchronizing on a Book instance.

Code Block
bgColor#FFCCCC

// Client
public class BookWrapper {
  private final Book book;

  BookWrapper(Book book) {
    this.book = book;
  }

  public void issue(int days) {
    book.issue(days);
  }

  public Calendar getDueDate() {
    return book.getDueDate();
  }

  public void renew() {
    synchronized(book) {
      if (book.getDueDate().before(Calendar.getInstance())) {
        throw new IllegalStateException("Book overdue");
      } else {
        book.issue(14); // Issue book for 14 days
      }
    }
  }
}

If the Book class were to change its synchronization policy in the future, the BookWrapper class's locking strategy might silently break. For instance, the BookWrapper class's locking strategy would break if Book were modified to use a private final lock object, as recommended by rule LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code. This is because threads that call BookWrapper.getDueDate() would perform operations on the thread-safe Book using its new locking policy. However, threads that call the renew() method would always synchronize on the intrinsic lock of the Book instance. Consequently, the implementation would use two different locks.

...

This compliant solution uses a private final lock object and synchronizes the methods of the BookWrapper class using this lock.:

Code Block
bgColor#ccccff

public final class BookWrapper {
  private final Book book;
  private final Object lock = new Object();

  BookWrapper(Book book) {
    this.book = book;
  }

  public void issue(int days) {
    synchronized(lock) {
      book.issue(days);
    }
  }

  public Calendar getDueDate() {
    synchronized(lock) {
      return book.getDueDate();
    }
  }

  public void renew() {
    synchronized(lock) {
      if (book.getDueDate().before(Calendar.getInstance())) {
        throw new IllegalStateException("Book overdue");
      } else {
        book.issue(14); // Issue book for 14 days
      }
    }
  }
}

...

Goetz and colleagues describe the fragility of class extension for adding functionality to to thread-safe classes [Goetz 2006]:

Extension is more fragile than adding code directly to a class, because the implementation of the synchronization policy is now distributed over multiple, separately maintained source files. If the underlying class were to change its synchronization policy by choosing a different lock to guard its state variables, the subclass would subtly and silently break because it no longer used the right lock to control concurrent access to the base class's state.

In this noncompliant code example, the PrintableIPAddressList class extends the thread-safe IPAddressList class. PrintableIPAddressList locks on IPAddressList.ips in the addAndPrintIPAddresses() method. This is another example of client-side locking because a subclass is using an object owned and locked by its superclass.

Code Block
bgColor#FFCCCC

// This class could change its locking policy in the future,
// for example, if new non-atomic methods are added
class IPAddressList {
  private final List<InetAddress> ips = 
      Collections.synchronizedList(new ArrayList<InetAddress>());

  public List<InetAddress> getList() {
    return ips; // No defensive copies required
                // as visibility is package-private
  }

  public void addIPAddress(InetAddress address) {
    ips.add(address);
  }
}

class PrintableIPAddressList extends IPAddressList {
  public void addAndPrintIPAddresses(InetAddress address) {
    synchronized (getList()) {
      addIPAddress(address);
      InetAddress[] ia =
          (InetAddress[]) getList().toArray(new InetAddress[0]);
      // ...
    }
  }
}

If the IPAddressList class were modified to use block block synchronization on a private final lock object, as recommended by rule LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code, the PrintableIPAddressList subclass would silently break. Moreover, if a wrapper such as Collections.synchronizedList() were used, it would be difficult for a client to determine the type of the class being wrapped to extend it [Goetz 2006].

...

This compliant solution wraps an object of the IPAddressList class and provides synchronized accessors to manipulate the state of the object. Composition offers encapsulation benefits, usually with minimal overhead . Refer (refer to rule OBJ02-J. Preserve dependencies in subclasses when changing superclasses for more information on composition).

Code Block
bgColor#ccccff

// Class IPAddressList remains unchanged
class PrintableIPAddressList {
  private final IPAddressList ips;

  public PrintableIPAddressList(IPAddressList list) {
    this.ips = list;
  }

  public synchronized void addIPAddress(InetAddress address) {
    ips.addIPAddress(address);
  }

  public synchronized void addAndPrintIPAddresses(InetAddress address) {
    addIPAddress(address);
    InetAddress[] ia =
        (InetAddress[]) ips.getList().toArray(new InetAddress[0]);
    // ...
  }
}

...

Using client-side locking when the the thread-safe class fails to commit to its locking strategy can cause data inconsistencies and deadlock.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

LCK11-J

low Low

probable Probable

medium Medium

P4

L3

Bibliography

[API 20062014]

Class Vector, Class WeakReference, Class Collections
Class ConcurrentHashMap<K,V>

Class Vector<E>
Class WeakReference<T>

[JavaThreads 2004]

Section 8.2, "Synchronization and Collection Classes"

[Goetz 2006]

Section 4.4.1, "Client-side Locking; "
Section 4.4.2, "Composition; and "
Section 5.2.1, "ConcurrentHashMap"

[Lee 2009]

"Map & Compound Operation"

 

...