Skip to end of metadata
Go to start of metadata

The Java Tutorials, Wrapper Implementations [Java Tutorials], 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 rather than synchronizing on the Collection view itself.

Disregarding this advice may result in nondeterministic behavior.

Any class that uses a collection view rather than 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 have accessible backing collections must synchronize on the backing collection; synchronization on the view is a violation of this rule.

Noncompliant Code Example (Collection View)

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

private final Map<Integer, String> mapView =
    Collections.synchronizedMap(new HashMap<Integer, String>());
private final Set<Integer> setView = mapView.keySet();

public Map<Integer, String> getMap() {
  return mapView;
}

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

In this example, HashMap provides the backing collection for the synchronized map represented by mapView, which provides the backing collection for setView, as shown in the following figure.

The HashMap object is inaccessible, but mapView is accessible via the public getMap() method. Because the synchronized statement uses the intrinsic lock of setView rather than of mapView, another thread can modify the synchronized map and invalidate the k iterator.

Compliant Solution (Collection Lock Object)

This compliant solution synchronizes on the mapView field rather than on the setView field:

private final Map<Integer, String> mapView =
    Collections.synchronizedMap(new HashMap<Integer, String>());
private final Set<Integer> setView = mapView.keySet();

public Map<Integer, String> getMap() {
  return mapView;
}

public void doSomething() {
  synchronized (mapView) {  // Synchronize on map, rather than set
    for (Integer k : setView) {
      // ...
    }
  }
}

This code is compliant because the map's underlying structure cannot be changed during the iteration.

Risk Assessment

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

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

LCK04-J

Low

Probable

Medium

P4

L3

Automated Detection

Some static analysis tools are capable of detecting violations of this rule.

ToolVersionCheckerDescription
ThreadSafe1.3

CCE_CC_SYNC_ON_VIEW
CCE_CC_ITER_VIEW_NO_LOCK
CCE_CC_ITER_VIEW_BOTH_LOCKS
CCE_CC_ITER_VIEW_WRONG_LOCK

Implemented

 

Bibliography

Issue Tracking

0%

Review List

  1. handler

    suggested => "HashMap is not accessible, but the Map view is. Because the set view is synchronized instead of the map view, another thread can modify the contents of map and invalidate the k iterator."

    Priority MEDIUM
    dmohindr
    Apr 09, 2010

 


9 Comments

  1. I prefer the previous title more because this title is misleading. There might be times when synchronization is required over the collection view as well.

    1. How about this as a title?

      CON40-J. Do not synchronize on a collection view while the original collection object is still accessible

      BTW I've removed the draft label et. al. as we obviously haven't finished this rule yet.

  2. The java.util.Collections interface's documentation API 06 warns about the consequences of synchronizing on any view over a collection object, rather than synchronizing on the collection object itself.

    API 06 does not say rather than...it just says you should do it.

    1. Do not synchronize on a collection view if the backing collection is accessible

      1. I've merged our suggestions & changed the title. Please add the 'draft' tag back if you think the rule is now OK.

        1. I don't think "still" is necessary in the title. Plus the text before the quote does not reflect what the API 06 ref says.

          1. I am experimenting with this issue tracking section where I can add issues once a guideline has gone past me. What do you think?

            1. Upon further reflection, I prefer my original title:

              CON40-J. Do not synchronize on a collection view

              as a situation where the original collection is not accessible to the JVM would have its own security implications. A likely scenario is that your code has access to a collection view but not the original collection. In that case you should not synch on the view as the original collection is (probably) still modifiable by some code...presumably the class that offered you the view. In that case I would argue to use CON04-J.

              WRT the API 06 ref, it leaves a lot of things implicit. I still think the text before the quote is a reasonable interpretation, but the point is not just to interpret the quote but to give general guideance beyond the quote.

              Dhruv, if you still disagree with me on either issue than let's talk in person; we're not going to settle it over the wiki.

              I'm not sure how to code this response using the issue tracking, so I'll just re-assign responsibility on the issues to you (smile)

              1. ^^ this issue has been resolved.