# HG changeset patch
# User shade
# Date 1450376098 -10800
# Thu Dec 17 21:14:58 2015 +0300
# Node ID ce72c7641f386038764e3be466cd82dd1e853d35
# Parent 59f1d54869b9b0709d6a1f363cb5270fa9cc674e
8145539: (coll) AbstractMap.keySet and .values should not be volatile
Reviewed-by: redestad, plevart, dl, psandoz
diff --git a/src/share/classes/java/util/AbstractMap.java b/src/share/classes/java/util/AbstractMap.java
--- a/src/share/classes/java/util/AbstractMap.java
+++ b/src/share/classes/java/util/AbstractMap.java
@@ -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.
+ *
+ * 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.
+ *
+ *
It is also imperative that implementations read the field only once,
+ * as in:
+ *
+ *
{@code
+ * public Set keySet() {
+ * Set ks = keySet; // single racy read
+ * if (ks == null) {
+ * ks = new KeySet();
+ * keySet = ks;
+ * }
+ * return ks;
+ * }
+ *}
*/
- transient volatile Set keySet;
- transient volatile Collection values;
+ transient Set keySet;
+ transient Collection values;
/**
* {@inheritDoc}
@@ -325,8 +344,9 @@
* method will not all return the same set.
*/
public Set keySet() {
- if (keySet == null) {
- keySet = new AbstractSet() {
+ Set ks = keySet;
+ if (ks == null) {
+ ks = new AbstractSet() {
public Iterator iterator() {
return new Iterator() {
private Iterator> 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 values() {
- if (values == null) {
- values = new AbstractCollection() {
+ Collection vals = values;
+ if (vals == null) {
+ vals = new AbstractCollection() {
public Iterator iterator() {
return new Iterator() {
private Iterator> i = entrySet().iterator();
@@ -418,8 +440,9 @@
return AbstractMap.this.containsValue(v);
}
};
+ values = vals;
}
- return values;
+ return vals;
}
public abstract Set> entrySet();
diff --git a/src/share/classes/java/util/EnumMap.java b/src/share/classes/java/util/EnumMap.java
--- a/src/share/classes/java/util/EnumMap.java
+++ b/src/share/classes/java/util/EnumMap.java
@@ -380,10 +380,11 @@
*/
public Set keySet() {
Set 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 {
@@ -418,10 +419,11 @@
*/
public Collection values() {
Collection 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 {
diff --git a/src/share/classes/java/util/HashMap.java b/src/share/classes/java/util/HashMap.java
--- a/src/share/classes/java/util/HashMap.java
+++ b/src/share/classes/java/util/HashMap.java
@@ -902,8 +902,12 @@
* @return a set view of the keys contained in this map
*/
public Set keySet() {
- Set ks;
- return (ks = keySet) == null ? (keySet = new KeySet()) : ks;
+ Set ks = keySet;
+ if (ks == null) {
+ ks = new KeySet();
+ keySet = ks;
+ }
+ return ks;
}
final class KeySet extends AbstractSet {
@@ -949,8 +953,12 @@
* @return a view of the values contained in this map
*/
public Collection values() {
- Collection vs;
- return (vs = values) == null ? (values = new Values()) : vs;
+ Collection vs = values;
+ if (vs == null) {
+ vs = new Values();
+ values = vs;
+ }
+ return vs;
}
final class Values extends AbstractCollection {
diff --git a/src/share/classes/java/util/IdentityHashMap.java b/src/share/classes/java/util/IdentityHashMap.java
--- a/src/share/classes/java/util/IdentityHashMap.java
+++ b/src/share/classes/java/util/IdentityHashMap.java
@@ -964,10 +964,11 @@
*/
public Set keySet() {
Set 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 {
@@ -1069,10 +1070,11 @@
*/
public Collection values() {
Collection 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 {
diff --git a/src/share/classes/java/util/LinkedHashMap.java b/src/share/classes/java/util/LinkedHashMap.java
--- a/src/share/classes/java/util/LinkedHashMap.java
+++ b/src/share/classes/java/util/LinkedHashMap.java
@@ -528,8 +528,12 @@
* @return a set view of the keys contained in this map
*/
public Set keySet() {
- Set ks;
- return (ks = keySet) == null ? (keySet = new LinkedKeySet()) : ks;
+ Set ks = keySet;
+ if (ks == null) {
+ ks = new LinkedKeySet();
+ keySet = ks;
+ }
+ return ks;
}
final class LinkedKeySet extends AbstractSet {
@@ -577,8 +581,12 @@
* @return a view of the values contained in this map
*/
public Collection values() {
- Collection vs;
- return (vs = values) == null ? (values = new LinkedValues()) : vs;
+ Collection vs = values;
+ if (vs == null) {
+ vs = new LinkedValues();
+ values = vs;
+ }
+ return vs;
}
final class LinkedValues extends AbstractCollection {
diff --git a/src/share/classes/java/util/TreeMap.java b/src/share/classes/java/util/TreeMap.java
--- a/src/share/classes/java/util/TreeMap.java
+++ b/src/share/classes/java/util/TreeMap.java
@@ -852,7 +852,11 @@
*/
public Collection values() {
Collection vs = values;
- return (vs != null) ? vs : (values = new Values());
+ if (vs == null) {
+ vs = new Values();
+ values = vs;
+ }
+ return vs;
}
/**
diff --git a/src/share/classes/java/util/WeakHashMap.java b/src/share/classes/java/util/WeakHashMap.java
--- a/src/share/classes/java/util/WeakHashMap.java
+++ b/src/share/classes/java/util/WeakHashMap.java
@@ -866,7 +866,11 @@
*/
public Set keySet() {
Set ks = keySet;
- return (ks != null ? ks : (keySet = new KeySet()));
+ if (ks == null) {
+ ks = new KeySet();
+ keySet = ks;
+ }
+ return ks;
}
private class KeySet extends AbstractSet {
@@ -915,7 +919,11 @@
*/
public Collection values() {
Collection vs = values;
- return (vs != null) ? vs : (values = new Values());
+ if (vs == null) {
+ vs = new Values();
+ values = vs;
+ }
+ return vs;
}
private class Values extends AbstractCollection {