Checker Concurrency   as of Julia version 2.5.0 (built on 4 Jul 2018)

belongs to group Basic

Identify basic concurrency errors


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.

Action: check if the warnings correspond to actual possible errors for a concurrent execution of the program.

Examples


Consider the following program:

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";
  private String lock3 = new String("lock");
  private String lock4 = new String("lock").intern();
  private volatile Map<String, Integer> map = new HashMap<>();

  private TestConcurrency() {}

  public static TestConcurrency getInstance1() {
    if (instance == null)
      instance = new TestConcurrency();

    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) {
      i++;
    }

    return i;
  }

  public int test1() {
    synchronized (lock2) {
      return next();
    }
  }

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

  public int test3() {
    synchronized (lock4) {
      return next();
    }
  }

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

  public int test6(String s) {
    synchronized (s) {
      return next();
    }
  }
}

This checker issues the following warnings:

TestConcurrency.java: [Concurrency: UselessVolatileModifierWarning] Volatile modifier is useless for field lock2 and should be removed for better efficiency
TestConcurrency.java: [Concurrency: UselessVolatileModifierWarning] Volatile modifier is useless for field map and should be removed for better efficiency
TestConcurrency.java: [Concurrency: VolatileContainerFieldWarning] Volatile container field map safely publishes the container reference, not its elements
TestConcurrency.java:16: [Concurrency: UnsafeLazyInitialisationWarning] Field instance seems lazily initialised in an incorrect way: you should synchronise on a static guard to avoid multiple concurrent initializations
TestConcurrency.java:48: [Concurrency: UselessSynchronizationWarning] Useless synchronization scope: it does not access the heap
TestConcurrency.java:56: [Concurrency: SynchronisationOnInternedStringWarning] Synchronization seems to occur on a string interned at line 7: this might lead to unexpected behaviors or even deadlock
TestConcurrency.java:68: [Concurrency: SynchronisationOnInternedStringWarning] Synchronization seems to occur on a string interned at line 9: this might lead to unexpected behaviors or even deadlock
TestConcurrency.java:78: [Concurrency: SynchronisationOnInternedStringWarning] Synchronization seems to occur on a string interned at line 7: this might lead to unexpected behaviors or even deadlock

Let us discuss the warnings above. 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 latter, here, has only the effect of making more expensive the access to those fields. The same modifier on field map seems actually to suggest that the goal of the programmer was to publish updates to the map, rather than the map itself. But this is not what the modifier accomplishes: a concurrent map should be used instead. Hence Julia issues the third warning. The warning at line 16 is due to the fact that, in a concurrent setting, the singleton pattern implemented in method getInstance1() is incorrect and might actually generate more than one instance. Its implementation in methods getInstance2() and getInstance3() is instead correct and Julia does not issue any warning there. (getInstance3() double-checks field instance to reduce the overhead of synchronization.) The synchronization at line 48 is useless since the critical region does not affect the heap: that synchronization is actually harmful, since it has a cost. The synchronizations at lines 56, 68 and 78 occur (also 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. Note that no such warning is issued inside method test2(), since it does not synchronize on an interned string.