LCK04-J. Do not synchronize on a collection view if the backing collection is accessible

The java.util.Collections interface's documentation [[API 2006]] warns about the consequences of failing to synchronize on an accessible collection object when iterating over its view:

It is imperative that the user manually synchronize on the returned map when iterating over any of its collection views... Failure to follow this advice may result in non-deterministic behavior.

Any class that uses a collection view as the lock object rather than using the backing collection as the lock object may end up with two distinct locking strategies. When the backing collection is accessible to multiple threads, the class that locked on the collection view has violated the thread-safety properties, and is unsafe. Consequently, programs that both require synchronization while iterating over collection views and also have accessible backing collections must synchronize on the backing collection; synchronization on the view is prohibited.

Noncompliant Code Example (Collection View)

This noncompliant code example creates two views: a synchronized view of an empty HashMap encapsulated by the map field and a set view of the map's keys encapsulated by the set field. This example synchronizes on the set view [[Tutorials 2008]].

// map has package-private accessibility
final Map<Integer, String> map = Collections.synchronizedMap(new HashMap<Integer, String>());
private final Set<Integer> set = map.keySet();

public void doSomething() {
  synchronized(set) {  // Incorrectly synchronizes on set
    for (Integer k : set) {
      // ...
    }
  }
}

In this example, HashMap provides the backing collection for Map, which provides the backing collection for Set, as shown in the following figure:

HashMap is inaccessible, but the map view is accessible. Because the set view is synchronized but the map view is unsynchronized, another thread can modify the contents of map and invalidate the k iterator.

Compliant Solution (Collection Lock Object)

This compliant solution synchronizes on the map view rather than on the set view.

// map has package-private accessibility
final Map<Integer, String> map = Collections.synchronizedMap(new HashMap<Integer, String>());
private final Set<Integer> set = map.keySet();

public void doSomething() {
  synchronized(map) {  // Synchronize on map, not set
    for (Integer k : set) {
      // ...
    }
  }
}

This code is compliant because the map's underlying structure cannot be changed while an iteration is in progress.

Risk Assessment

Synchronizing on a collection view instead of the collection object can cause nondeterministic behavior.

Related Vulnerabilities

Any vulnerabilities resulting from the violation of this rule are listed on the CERT website.

Bibliography

Issue Tracking

null      [!The CERT Oracle Secure Coding Standard for Java^button_arrow_up.png!]      null