CON31-J. Avoid client-side locking when using classes that do not commit to their locking strategy

"Client-side locking entails guarding client code that uses some object X with the lock X uses to guard its own state. In order to use client-side locking, you must know what lock X uses." [[Goetz 06]].

While client-side locking is acceptable if the thread-safe class commits to its locking strategy and clearly documents it, Goetz et al. [[Goetz 06]] caution against its misuse:

If extending a class to add another atomic operation is fragile because it distributes the locking code for a class over multiple classes in an object hierarchy, client-side locking is even more fragile because it entails putting locking code for class C into classes that are totally unrelated to C. Exercise care when using client-side locking on classes that do not commit to their locking strategy.

The documentation of a class that does not support client-side locking should explicitly state its applicability. An example of when not to use client-side locking is the class java.util.concurrent.ConcurrentHashMap<K,V>, whose documentation states [[API 06]]:

... 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.

In general, use client-side locking only when the documentation of the class recommends it. For example, the documentation of the wrapper method synchronizedList() of class java.util.Collections 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.

Noncompliant Code Example

This noncompliant code example uses a thread-safe class Book that cannot be modified.

final class Book {
  // May change its locking policy in the future to use private internal 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(Calendar.DAY_OF_MONTH, days);	 
  }

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

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

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

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

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

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

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

If class Book changes its synchronization policy in the future, the BookWrapper class's locking strategy might silently break. For instance, the Bookwrapper locking strategy will definitely break if Book is modified to use an internal private lock, as recommended by CON04-J. Synchronize using an internal private final lock object. This is because threads that call getDueDate() of class BookWrapper may perform operations on the thread-safe Book using its new locking policy, however, threads that call method renew() will always synchronize on the intrinsic lock of the Book instance. Consequently, the implementation will use two different locks.

Compliant Solution (composition)

This compliant solution uses an internal private lock object and synchronizes all its methods using this lock.

public 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();
    }
  }

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

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

Consequently, its locking strategy is independent of the locking policy of the Book instance. This solution incurs a very small performance penalty but the resulting code is much more robust [[Goetz 06]].

Risk Assessment

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

Automated Detection

TODO

Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this rule on the CERT website.

References

[[API 06]] Class Vector, Class WeakReference, Class ConcurrentHashMap<K,V>
[[JavaThreads 04]] 8.2 "Synchronization and Collection Classes"
[[Goetz 06]] 4.4.1. Client-side Locking, 4.4.2. Composition and 5.2.1. ConcurrentHashMap
[[Lee 09]] "Map & Compound Operation"

VOID CON06-J. Do not defer a thread that is holding a lock      11. Concurrency (CON)