8145539: (coll) AbstractMap.keySet and .values should not be volatile
authorshade
Thu, 17 Dec 2015 21:14:58 +0300
changeset 34712 b183cfd1ce17
parent 34711 65544417508e
child 34713 a89d6429ed35
8145539: (coll) AbstractMap.keySet and .values should not be volatile Reviewed-by: redestad, plevart, dl, psandoz
jdk/src/java.base/share/classes/java/util/AbstractMap.java
jdk/src/java.base/share/classes/java/util/EnumMap.java
jdk/src/java.base/share/classes/java/util/HashMap.java
jdk/src/java.base/share/classes/java/util/IdentityHashMap.java
jdk/src/java.base/share/classes/java/util/LinkedHashMap.java
jdk/src/java.base/share/classes/java/util/TreeMap.java
jdk/src/java.base/share/classes/java/util/WeakHashMap.java
--- a/jdk/src/java.base/share/classes/java/util/AbstractMap.java	Thu Dec 17 20:42:01 2015 +0300
+++ b/jdk/src/java.base/share/classes/java/util/AbstractMap.java	Thu Dec 17 21:14:58 2015 +0300
@@ -304,9 +304,28 @@
      * Each of these fields are initialized to contain an instance of the
      * appropriate view the first time this view is requested.  The views are
      * stateless, so there's no reason to create more than one of each.
+     *
+     * <p>Since there is no synchronization performed while accessing these fields,
+     * it is expected that java.util.Map view classes using these fields have
+     * no non-final fields (or any fields at all except for outer-this). Adhering
+     * to this rule would make the races on these fields benign.
+     *
+     * <p>It is also imperative that implementations read the field only once,
+     * as in:
+     *
+     * <pre> {@code
+     * public Set<K> keySet() {
+     *   Set<K> ks = keySet;  // single racy read
+     *   if (ks == null) {
+     *     ks = new KeySet();
+     *     keySet = ks;
+     *   }
+     *   return ks;
+     * }
+     *}</pre>
      */
-    transient volatile Set<K>        keySet;
-    transient volatile Collection<V> values;
+    transient Set<K>        keySet;
+    transient Collection<V> values;
 
     /**
      * {@inheritDoc}
@@ -325,8 +344,9 @@
      * method will not all return the same set.
      */
     public Set<K> keySet() {
-        if (keySet == null) {
-            keySet = new AbstractSet<K>() {
+        Set<K> ks = keySet;
+        if (ks == null) {
+            ks = new AbstractSet<K>() {
                 public Iterator<K> iterator() {
                     return new Iterator<K>() {
                         private Iterator<Entry<K,V>> i = entrySet().iterator();
@@ -361,8 +381,9 @@
                     return AbstractMap.this.containsKey(k);
                 }
             };
+            keySet = ks;
         }
-        return keySet;
+        return ks;
     }
 
     /**
@@ -382,8 +403,9 @@
      * method will not all return the same collection.
      */
     public Collection<V> values() {
-        if (values == null) {
-            values = new AbstractCollection<V>() {
+        Collection<V> vals = values;
+        if (vals == null) {
+            vals = new AbstractCollection<V>() {
                 public Iterator<V> iterator() {
                     return new Iterator<V>() {
                         private Iterator<Entry<K,V>> i = entrySet().iterator();
@@ -418,8 +440,9 @@
                     return AbstractMap.this.containsValue(v);
                 }
             };
+            values = vals;
         }
-        return values;
+        return vals;
     }
 
     public abstract Set<Entry<K,V>> entrySet();
--- a/jdk/src/java.base/share/classes/java/util/EnumMap.java	Thu Dec 17 20:42:01 2015 +0300
+++ b/jdk/src/java.base/share/classes/java/util/EnumMap.java	Thu Dec 17 21:14:58 2015 +0300
@@ -380,10 +380,11 @@
      */
     public Set<K> keySet() {
         Set<K> ks = keySet;
-        if (ks != null)
-            return ks;
-        else
-            return keySet = new KeySet();
+        if (ks == null) {
+            ks = new KeySet();
+            keySet = ks;
+        }
+        return ks;
     }
 
     private class KeySet extends AbstractSet<K> {
@@ -418,10 +419,11 @@
      */
     public Collection<V> values() {
         Collection<V> vs = values;
-        if (vs != null)
-            return vs;
-        else
-            return values = new Values();
+        if (vs == null) {
+            vs = new Values();
+            values = vs;
+        }
+        return vs;
     }
 
     private class Values extends AbstractCollection<V> {
--- a/jdk/src/java.base/share/classes/java/util/HashMap.java	Thu Dec 17 20:42:01 2015 +0300
+++ b/jdk/src/java.base/share/classes/java/util/HashMap.java	Thu Dec 17 21:14:58 2015 +0300
@@ -902,8 +902,12 @@
      * @return a set view of the keys contained in this map
      */
     public Set<K> keySet() {
-        Set<K> ks;
-        return (ks = keySet) == null ? (keySet = new KeySet()) : ks;
+        Set<K> ks = keySet;
+        if (ks == null) {
+            ks = new KeySet();
+            keySet = ks;
+        }
+        return ks;
     }
 
     final class KeySet extends AbstractSet<K> {
@@ -949,8 +953,12 @@
      * @return a view of the values contained in this map
      */
     public Collection<V> values() {
-        Collection<V> vs;
-        return (vs = values) == null ? (values = new Values()) : vs;
+        Collection<V> vs = values;
+        if (vs == null) {
+            vs = new Values();
+            values = vs;
+        }
+        return vs;
     }
 
     final class Values extends AbstractCollection<V> {
--- a/jdk/src/java.base/share/classes/java/util/IdentityHashMap.java	Thu Dec 17 20:42:01 2015 +0300
+++ b/jdk/src/java.base/share/classes/java/util/IdentityHashMap.java	Thu Dec 17 21:14:58 2015 +0300
@@ -964,10 +964,11 @@
      */
     public Set<K> keySet() {
         Set<K> ks = keySet;
-        if (ks != null)
-            return ks;
-        else
-            return keySet = new KeySet();
+        if (ks == null) {
+            ks = new KeySet();
+            keySet = ks;
+        }
+        return ks;
     }
 
     private class KeySet extends AbstractSet<K> {
@@ -1069,10 +1070,11 @@
      */
     public Collection<V> values() {
         Collection<V> vs = values;
-        if (vs != null)
-            return vs;
-        else
-            return values = new Values();
+        if (vs == null) {
+            vs = new Values();
+            values = vs;
+        }
+        return vs;
     }
 
     private class Values extends AbstractCollection<V> {
--- a/jdk/src/java.base/share/classes/java/util/LinkedHashMap.java	Thu Dec 17 20:42:01 2015 +0300
+++ b/jdk/src/java.base/share/classes/java/util/LinkedHashMap.java	Thu Dec 17 21:14:58 2015 +0300
@@ -528,8 +528,12 @@
      * @return a set view of the keys contained in this map
      */
     public Set<K> keySet() {
-        Set<K> ks;
-        return (ks = keySet) == null ? (keySet = new LinkedKeySet()) : ks;
+        Set<K> ks = keySet;
+        if (ks == null) {
+            ks = new LinkedKeySet();
+            keySet = ks;
+        }
+        return ks;
     }
 
     final class LinkedKeySet extends AbstractSet<K> {
@@ -577,8 +581,12 @@
      * @return a view of the values contained in this map
      */
     public Collection<V> values() {
-        Collection<V> vs;
-        return (vs = values) == null ? (values = new LinkedValues()) : vs;
+        Collection<V> vs = values;
+        if (vs == null) {
+            vs = new LinkedValues();
+            values = vs;
+        }
+        return vs;
     }
 
     final class LinkedValues extends AbstractCollection<V> {
--- a/jdk/src/java.base/share/classes/java/util/TreeMap.java	Thu Dec 17 20:42:01 2015 +0300
+++ b/jdk/src/java.base/share/classes/java/util/TreeMap.java	Thu Dec 17 21:14:58 2015 +0300
@@ -852,7 +852,11 @@
      */
     public Collection<V> values() {
         Collection<V> vs = values;
-        return (vs != null) ? vs : (values = new Values());
+        if (vs == null) {
+            vs = new Values();
+            values = vs;
+        }
+        return vs;
     }
 
     /**
--- a/jdk/src/java.base/share/classes/java/util/WeakHashMap.java	Thu Dec 17 20:42:01 2015 +0300
+++ b/jdk/src/java.base/share/classes/java/util/WeakHashMap.java	Thu Dec 17 21:14:58 2015 +0300
@@ -866,7 +866,11 @@
      */
     public Set<K> keySet() {
         Set<K> ks = keySet;
-        return (ks != null ? ks : (keySet = new KeySet()));
+        if (ks == null) {
+            ks = new KeySet();
+            keySet = ks;
+        }
+        return ks;
     }
 
     private class KeySet extends AbstractSet<K> {
@@ -915,7 +919,11 @@
      */
     public Collection<V> values() {
         Collection<V> vs = values;
-        return (vs != null) ? vs : (values = new Values());
+        if (vs == null) {
+            vs = new Values();
+            values = vs;
+        }
+        return vs;
     }
 
     private class Values extends AbstractCollection<V> {