Checker Concurrency

belongs to group Basic
Identify basic concurrency errors

Frameworks supported by this checker

  • java up to 11
  • android up to API level 28
  • dotnet

Warnings generated by this checker

  • BlockingCallInsideSynchronizationWarning: a blocking call occurs inside a synchronization block, hence increasing monitor contention and reducing performance [ CWE833 ]
  • ExpensiveSynchronizationOnStaticWarning: a synchronized statement on a static guard should lock an instance guard instead [ CWE413 ]
  • ImpossibleClientSideLockingWarning: synchronisation occurs on a concurrent map that does not allow client-side locking [ CWE413 ]
  • SynchronisationOnInternedStringWarning: synchronisation occurs on an interned string [ CWE412 ]
  • SynchronousCallToThreadBodyWarning: the body of a thread is called synchronously [ CWE572 ]
  • UnsafeLazyInitialisationWarning: a static field is lazily initialized in an incorrect way [ CWE609 ]
  • UselessSynchronizationWarning: a synchronized statement is useless [ CWE585 ]
  • UselessVolatileModifierWarning: a field should not be declared volatile [ CWE662 ]
  • VolatileArrayFieldWarning: an array field has been declared volatile [ CWE567 ]
  • VolatileContainerFieldWarning: a container field has been declared volatile [ CWE567 ]

Options accepted by this checker

  • none

Annotations understood by this checker

  • @com.juliasoft.julia.checkers.guardedBy.GetsLocked
  • @com.juliasoft.julia.checkers.guardedBy.GetsUnlocked
  • @com.juliasoft.julia.checkers.concurrency.MonitorEnter
  • @com.juliasoft.julia.checkers.concurrency.MonitorExit


Description

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.

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

Consider the following program:

using System;
using System.Collections;
using System.Collections.Generic;

namespace Concurrency 
{
	public class Concurrency
    {
        private static Concurrency instance;
        private static readonly object lock1 = new object();
        private volatile string lock2 = "lock"; 
        private string lock3 = new string("lock".ToCharArray());
        private string lock4 = string.Intern(new string("lock".ToCharArray()));
        private volatile Dictionary<string, int> map = new Dictionary<string, int>(); 
        private Concurrency() { }
        public static Concurrency GetInstance1()
        {
            if (instance == null)
                instance = new Concurrency(); 
            return instance;
        }
        public static Concurrency GetInstance2()
        {
            lock (lock1)
            {
                if (instance == null)
                    instance = new Concurrency();
            }
            return instance;
        }
        public static Concurrency GetInstance3()
        {
            if (instance == null)
                lock (lock1)
                {
                    if (instance == null)
                        instance = new Concurrency();
                }
            return instance;
        }
        private int counter;
        private int Next()
        {
            map.Add((++counter).ToString(), counter);
            return counter;
        }
        public int Step(int i)
        {
            lock (lock1)
            {
                i++;
            }
            return i;
        }
        public int Test1()
        {
            lock (lock2)
            {
                return Next();
            }
        }
        public int Test2()
        {
            lock (lock3)
            {
                return Next();
            }
        }
        public int Test3()
        {
            lock (lock4) 
            {
                return Next();
            }
        }
        public int CallTest6()
        {
            return Test6(lock2);
        }
        public int Test6(string s)
        {
            lock (s) 
            {
                return Next();
            }
        }
    }
}

This checker issues the following warnings:

TestConcurrency.cs: [Concurrency: UselessVolatileModifierWarning] Volatile modifier is useless for field lock2 and should be removed for better efficiency
TestConcurrency.cs: [Concurrency: UselessVolatileModifierWarning] Volatile modifier is useless for field map and should be removed for better efficiency
TestConcurrency.cs: [Concurrency: VolatileContainerFieldWarning] Volatile container field map safely publishes the container reference, not its elements
TestConcurrency.cs:19: [Concurrency: UnsafeLazyInitialisationWarning] Field instance seems lazily initialised in an incorrect way: you should synchronise on a static guard to avoid multiple concurrent initializations
TestConcurrency.cs:49: [Concurrency: UselessSynchronizationWarning] Useless synchronization scope: it does not access the heap
TestConcurrency.cs:57: [Concurrency: SynchronisationOnInternedStringWarning] Synchronization seems to occur on a string interned at line 11: this might lead to unexpected behaviors or even deadlock
TestConcurrency.cs:71: [Concurrency: SynchronisationOnInternedStringWarning] Synchronization seems to occur on a string interned at line 13: this might lead to unexpected behaviors or even deadlock
TestConcurrency.cs:82: [Concurrency: SynchronisationOnInternedStringWarning] Synchronization seems to occur on a string interned at line 11: 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 19 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 49 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 57, 71 and 82 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.