8145539: (coll) AbstractMap.keySet and .values should not be volatile
Reviewed-by: redestad, plevart, dl, psandoz
--- 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> {