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.

Noncompliant Code Example

This noncompliant code example shows a thread-safe class Book. However, the class does not commit to its locking strategy and does not document that callers can safely use client-side locking. The class CheckOut uses client-side locking by synchronizing on a Book instance.

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, int days) {
    this.title = title; 
  }
  
  public synchronized void issueBook(int days) {
    dateIssued = Calendar.getInstance();
    dateDue = Calendar.getInstance();
    dateDue.add(Calendar.DAY_OF_MONTH, days);	 
  }

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

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

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

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

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

If class Book changes its synchronization policy in the future, the CheckOut class's locking strategy will silently break. This is because threads that call getDueDate() of class CheckOut may perform operations on the thread-safe Book using its new locking policy (internal private lock), however, threads that call method renewBook() 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 CheckOut {
  private final Book book;
  private final Object lock = new Object();

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

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

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

Consequently, its locking strategy is independent of the locking policy of the Book instance.

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
[[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)