Synchronizing on the return value of the Object.getClass() method can lead to unexpected behavior. Whenever the implementing class is subclassed, the subclass locks on the subclass's type. The Class object of the subclass is entirely distinct from the Class object of the parent class.

According to The Java Language Specification, §4.3.2, "The Class Object" [JLS 2005]:

A class method that is declared synchronized synchronizes on the lock associated with the Class object of the class.

Programmers who interpret this to mean that a subclass using getClass() will synchronize on the Class object of the base class are incorrect. The subclass will actually lock on its own Class object, which may or may not be what the programmer intended. Consequently, programs must not synchronize on the class object returned by getClass().

The programmer's actual intent should be clearly documented or annotated. Note that when a subclass fails to override an accessible noncompliant superclass's method, it inherits the method, which may lead to the false conclusion that the superclass's intrinsic lock is available in the subclass.

When synchronizing on a class literal, the corresponding lock object should be inaccessible to untrusted code. Callers from other packages cannot access class objects that are package-private; consequently, synchronizing on the intrinsic lock object of such classes is permitted (see LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code for more information).

Noncompliant Code Example (getClass() Lock Object)

In this noncompliant code example, the parse() method of the Base class parses a date and synchronizes on the class object returned by getClass(). The Derived class also inherits the parse() method. However, this inherited method synchronizes on Derived's class object because the inherited parse method's invocation of getClass() is really an invocation of this.getClass(), and the this argument is a reference to the instance of the Derived class.

The Derived class also adds a doSomethingAndParse() method that locks on the class object of the Base class because the developer misconstrued that the parse() method in Base always obtains a lock on the Base class object, and doSomethingAndParse() must follow the same locking policy. Consequently, the Derived class has two different locking strategies and fails to be thread-safe.

class Base {
  static DateFormat format =
      DateFormat.getDateInstance(DateFormat.MEDIUM);

  public Date parse(String str) throws ParseException {
    synchronized (getClass()) {
      return format.parse(str);
    }
  }
}

class Derived extends Base {
  public Date doSomethingAndParse(String str) throws ParseException {
    synchronized (Base.class) {
      // ...
      return format.parse(str);
    }
  }
}

Compliant Solution (Class Name Qualification)

In this compliant solution, the class name providing the lock (Base) is fully qualified:

class Base {
  static DateFormat format =
      DateFormat.getDateInstance(DateFormat.MEDIUM);

  public Date parse(String str) throws ParseException {
    synchronized (Base.class) {
      return format.parse(str);
    }
  }
}

// ...

This code example always synchronizes on the Base.class object, even when it is called from a Derived object.

Compliant Solution (Class.forName())

This compliant solution uses the Class.forName() method to synchronize on the Base class's Class object:

class Base {
  static DateFormat format =
      DateFormat.getDateInstance(DateFormat.MEDIUM);

  public Date parse(String str) throws ParseException {
    try {
      synchronized (Class.forName("Base")) {
        return format.parse(str);
      }
    } catch (ClassNotFoundException x) {
      // "Base" not found; handle error
    }
    return null;
  } 
}

// ...

Never accept untrusted inputs as arguments while loading classes using Class.forName() (see SEC03-J. Do not load trusted classes after allowing untrusted code to load arbitrary classes for more information).

Noncompliant Code Example (getClass() Lock Object, Inner Class)

This noncompliant code example synchronizes on the class object returned by getClass() in the parse() method of class Base. The Base class also has a nested Helper class whose doSomethingAndParse() method incorrectly synchronizes on the value returned by getClass().

class Base {
  static DateFormat format =
      DateFormat.getDateInstance(DateFormat.MEDIUM);

  public Date parse(String str) throws ParseException {
    synchronized (getClass()) { // Intend to synchronizes on Base.class
      return format.parse(str);
    }
  }

  public Date doSomething(String str) throws ParseException {
    return new Helper().doSomethingAndParse(str);
  }

  private class Helper {
    public Date doSomethingAndParse(String str) throws ParseException {
      synchronized (getClass()) { // Synchronizes on Helper.class
        // ...
        return format.parse(str);
      }
    }
  }
}

The call to getClass() in the Helper class returns a Helper class object instead of the Base class object. Consequently, a thread that calls Base.parse() locks on a different object than a thread that calls Base.doSomething(). It is easy to overlook concurrency errors in inner classes because they exist within the body of the containing outer class. A reviewer might incorrectly assume that the two classes have the same locking strategy.

Compliant Solution (Class Name Qualification)

This compliant solution synchronizes using a Base class literal in the parse() and doSomethingAndParse() methods:

class Base {
  // ...

  public Date parse(String str) throws ParseException {
    synchronized (Base.class) {
      return format.parse(str);
    }
  }

  private class Helper {
    public Date doSomethingAndParse(String str) throws ParseException {
      synchronized (Base.class) { // Synchronizes on Base class literal
        // ...
        return format.parse(str);
      }
    }
  }
}

Consequently, both Base and Helper lock on Base's intrinsic lock. Similarly, the Class.forName() method can be used instead of a class literal.

Risk Assessment

Synchronizing on the class object returned by getClass() can result in nondeterministic behavior.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

LCK02-J

Medium

Probable

Medium

P8

L2

Automated Detection

Some static analysis tools can detect violations of this rule

ToolVersionCheckerDescription
Parasoft Jtest

2023.1

CERT.LCK02.SGCDo not synchronize on the class object returned by the 'getClass' method
SonarQube
9.9

S3067




Bibliography



5 Comments

  1. Thanks for the notice.

    The 1st NCCE is technically correct, but could use some more. A routine that relies on the (un)thread-safety of Derived, for instance. Two threads that call Derived.doSomethingAndParse() and Derived.parse(), and generate a race condition. Unfortunately, this is starting to strain the abstraction, as this code doesn't have a real-world purpose. As it is, the NCCE is considerably more complex than before, but it is not more intuitive.

    1. I had also put a hidden main method which does exactly what you suggest wrt two threads. I think we can forgo that code and if required just say that two threads may call x and y methods and the end result is z. This is not necessary though.

      I am not sure why the code doesn't have any real world purpose or why it is not intuitive (this seems to be a conceivable mistake, in fact, a static field helps). Perhaps I didn't understand the previous code because it wasn't fully complete. Let me know if I missed something. Thanks.

      1. I don't understand the last change. Both NCEs should use getClass().

  2. I ran in to this today when my IDE highlighted some code I wrote; in my case locking on the child class object is exactly what I intended.

    I have a generic base class which provides functionality related to ThreadLocal, and I want to make sure that child classes deal only with a single ThreadLocal even if they forget to implement the singleton pattern as recommended in my documentation. To accomplish this, I synchronize on this.getClass() in the constructor while checking for / adding to a Map from child class to threadlocal instance. It's no problem if multiple distinct callers interact with my Map at the same time, so locking on my base class would introduce needless contention.

    private static final Map<Class<? extends AbstractContext>, ThreadLocal> THREAD_LOCAL_MAP = new HashMap<>();
    protected ThreadLocalContext() {
    // below code violates this rule despite not having a logical flaw
    synchronized(this.getClass()) {
    ThreadLocal<T> mappedThreadLocal = THREAD_LOCAL_MAP.get(this.getClass());
    if (mappedThreadLocal == null) {
    threadLocal = new ThreadLocal<>();
    THREAD_LOCAL_MAP.put(this.getClass(), threadLocal);
    } else {
    threadLocal = mappedThreadLocal;
    }
    }
    }


    1. The main danger is that you synchronize on the wrong Class instance, which is quite likely if your thread-local class has its own subclass hierarchy.  If you do not synchronize on a private final lock object (as recommended in LCK00-J() then ideally your code should have comments explaining why not.