JAVA.CONCURRENCY.LOCK.ISTR : Synchronization on Interned String (Java)

Summary

Synchronisation occurs on an interned string.

Concurrency is an important but complex aspect of modern software. As a consequence, it is often used in an incorrect way, also because the subtleties of the Java memory model are not always understood. This checker identifies a large class of common programming errors due to incorrect uses of concurrency primitives, such as incorrect implementations of the singleton pattern and incorrect uses of the volatile field modifier, whose goal is to publish a field update to all executing cores. The latter, however, has a cost in terms of execution time.

Properties

WARNING_FILTER += discard class="Synchronization on Interned String (Java)"

Example

import java.util.HashMap;
import java.util.Map;

public class TestConcurrency {
  private static TestConcurrency instance;
  private final static Object lock1 = new Object();
  private volatile String lock2 = "lock";                      // Useless volatile Modifier (Java) warning issued here
  private String lock3 = new String("lock");
  private String lock4 = new String("lock").intern();
  private volatile Map<String, Integer> map = new HashMap<>(); // Useless volatile Modifier (Java) warning issued here (multiple instances)

  private TestConcurrency() {}

  public static TestConcurrency getInstance1() {
      if (instance == null)
          instance = new TestConcurrency(); // Double-Checked Locking (Java) warning issued here (Java)
      return instance;
  }

  public static TestConcurrency getInstance2() {
      synchronized (lock1) {
          if (instance == null)
              instance = new TestConcurrency();
      }
      return instance;
  }

  public static TestConcurrency getInstance3() {
      if (instance == null)
          synchronized (lock1) {
              if (instance == null)
                  instance = new TestConcurrency();
          }
      return instance;
  }

  private int counter;

  private int next() {
      map.put(String.valueOf(++counter), counter);
      return counter;
  }

  public int step(int i) {
      synchronized (lock1) {              // Useless Synchronization (Java) warning issued here
          i++;
      }
      return i;
  }

  public int test1() {
      synchronized (lock2) {              // Synchronization on Interned String (Java) warning issued here
          return next();
      }
  }

  public int test2() {
      synchronized (lock3) {
          return next();
      }
  }

  public int test3() {
      synchronized (lock4) {              // Synchronization on Interned String (Java) warning issued here
          return next();
      }
  }

  public int callTest6() {
      return test6(lock2);
  }

  public int test6(String s) {
      synchronized (s) {                   // Synchronization on Interned String (Java) warning issued here
          return next();
      }
  }
}

• The volatile modifier for fields lock2 and map is useless, since those fields are only assigned in the constructor (implicitly) and consequently safely published at the end of the constructor: there is no need to force publication at every write operation, which is the semantics of the volatile modifier. The only effect of the volatile modifier in this case is to make field access more expensive.
• Specifying map as volatile seems actually to suggest that the goal of the programmer was to publish updates to the map, rather than the map itself. However, this is not what the modifier accomplishes, so a second instance of Useless volatile Modifier (Java) is issued.

  To publish updates to the map, use a concurrent map instead.

• In a concurrent setting, the singleton pattern implemented in method getInstance1() is incorrect and might actually generate more than one instance.

  The implementations in methods getInstance2() and getInstance3() are correct, so no warnings are issued for those methods.

• The synchronization in test1() is useless since the critical region does not affect the heap: that synchronization is actually harmful, since it has a cost.
• The synchronizations in test1(), test3() and test6() occur on interned strings. This is dangerous, since interned strings are unique objects across the whole program and synchronizations on them might lead the program into deadlock or useless thread blocking.

  No such warning is issued inside method test2(), since it does not synchronize on an interned string.

Resolution

Check if the warnings correspond to actual possible errors for a concurrent execution of the program.

Relevant Configuration File Parameters

The following configuration file parameters affect checks for this warning class.

• REPORT_SIMILAR_WARNINGS
• WARNING_FILTER