From 3d6ca162672f576b60d079c2b0f6dbf545d0c756 Mon Sep 17 00:00:00 2001 From: Michael Zangl Date: Sat, 2 Jan 2016 14:14:56 +0100 Subject: [PATCH 1/2] Fixed keys array access on some places --- .../josm/data/osm/AbstractPrimitive.java | 69 +++++++++++++++------- .../openstreetmap/josm/data/osm/OsmPrimitive.java | 36 +++++------ 2 files changed, 65 insertions(+), 40 deletions(-) diff --git a/src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java b/src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java index bb7127c..c716f23 100644 --- a/src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java +++ b/src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java @@ -481,14 +481,15 @@ public abstract class AbstractPrimitive implements IPrimitive { * Keys handling ------------*/ - // Note that all methods that read keys first make local copy of keys array reference. This is to ensure thread safety - reading - // doesn't have to be locked so it's possible that keys array will be modified. But all write methods make copy of keys array so - // the array itself will be never modified - only reference will be changed - /** * The key/value list for this primitive. + *

+ * Note that the keys field is synchronized using RCU. + * Writes to it are not synchronized by this object, the writers have to synchronize writes themselves. + *

+ * In short this means that you should not rely on this variable being the same value when read again and your should always copy it on writes. */ - protected String[] keys; + protected volatile String[] keys; /** * Replies the map of key/value pairs. Never replies null. The map can be empty, though. @@ -530,6 +531,8 @@ public abstract class AbstractPrimitive implements IPrimitive { * Sets the keys of this primitives to the key/value pairs in keys. * Old key/value pairs are removed. * If keys is null, clears existing key/value pairs. + *

+ * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used from multiple threads. * * @param keys the key/value pairs to set. If null, removes all existing key/value pairs. */ @@ -554,6 +557,8 @@ public abstract class AbstractPrimitive implements IPrimitive { /** * Set the given value to the given key. If key is null, does nothing. If value is null, * removes the key and behaves like {@link #remove(String)}. + *

+ * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used from multiple threads. * * @param key The key, for which the value is to be set. Can be null or empty, does nothing in this case. * @param value The value for the key. If null, removes the respective key/value pair. @@ -571,24 +576,47 @@ public abstract class AbstractPrimitive implements IPrimitive { keys = new String[] {key, value}; keysChangedImpl(originalKeys); } else { - for (int i = 0; i < keys.length; i += 2) { - if (keys[i].equals(key)) { - // This modifies the keys array but it doesn't make it invalidate for any time so its ok (see note no top) - keys[i+1] = value; - keysChangedImpl(originalKeys); - return; - } + int keyIndex = indexOfKey(keys, key); + int tagArrayLength = keys.length; + if (keyIndex < 0) { + keyIndex = tagArrayLength; + tagArrayLength += 2; } - String[] newKeys = Arrays.copyOf(keys, keys.length + 2); - newKeys[keys.length] = key; - newKeys[keys.length + 1] = value; + + // Do not try to optimize this array creation if the key already exists. + // We would need to convert the keys array to be an AtomicReferenceArray + // Or we would at least need a volatile write after the array was modified to + // ensure that changes are visible by other threads. + String[] newKeys = Arrays.copyOf(keys, tagArrayLength); + newKeys[keyIndex] = key; + newKeys[keyIndex + 1] = value; keys = newKeys; keysChangedImpl(originalKeys); } } /** + * Scans a key/value array for a given key. + * @param keys The key array. It is not modified. It may be null to indicate an emtpy array. + * @param key The key to search for. + * @return The position of that key in the keys array - which is always a multiple of 2 - or -1 if it was not found. + */ + private static int indexOfKey(String[] keys, String key) { + if (keys == null) { + return -1; + } + for (int i = 0; i < keys.length; i += 2) { + if (keys[i].equals(key)) { + return i; + } + } + return -1; + } + + /** * Remove the given key from the list + *

+ * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used from multiple threads. * * @param key the key to be removed. Ignored, if key is null. */ @@ -617,6 +645,8 @@ public abstract class AbstractPrimitive implements IPrimitive { /** * Removes all keys from this primitive. + *

+ * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used from multiple threads. */ @Override public void removeAll() { @@ -680,6 +710,7 @@ public abstract class AbstractPrimitive implements IPrimitive { } public final int getNumKeys() { + String[] keys = this.keys; return keys == null ? 0 : keys.length / 2; } @@ -718,13 +749,7 @@ public abstract class AbstractPrimitive implements IPrimitive { * @return true, if his primitive has a tag with key key */ public boolean hasKey(String key) { - String[] keys = this.keys; - if (key == null) return false; - if (keys == null) return false; - for (int i = 0; i < keys.length; i += 2) { - if (keys[i].equals(key)) return true; - } - return false; + return key != null && indexOfKey(keys, key) >= 0; } /** diff --git a/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java b/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java index 9d5cb5d..b90e896 100644 --- a/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java +++ b/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java @@ -775,7 +775,7 @@ public abstract class OsmPrimitive extends AbstractPrimitive implements Comparab /** * Returns {@link #getKeys()} for which {@code key} does not fulfill {@link #isUninterestingKey}. - * @return list of interesting tags + * @return A map of interesting tags */ public Map getInterestingTags() { Map result = new HashMap<>(); @@ -832,26 +832,22 @@ public abstract class OsmPrimitive extends AbstractPrimitive implements Comparab } private void updateTagged() { - if (keys != null) { - for (String key: keySet()) { - // 'area' is not really uninteresting (putting it in that list may have unpredictable side effects) - // but it's clearly not enough to consider an object as tagged (see #9261) - if (!isUninterestingKey(key) && !"area".equals(key)) { - updateFlagsNoLock(FLAG_TAGGED, true); - return; - } + for (String key: keySet()) { + // 'area' is not really uninteresting (putting it in that list may have unpredictable side effects) + // but it's clearly not enough to consider an object as tagged (see #9261) + if (!isUninterestingKey(key) && !"area".equals(key)) { + updateFlagsNoLock(FLAG_TAGGED, true); + return; } } updateFlagsNoLock(FLAG_TAGGED, false); } private void updateAnnotated() { - if (keys != null) { - for (String key: keySet()) { - if (getWorkInProgressKeys().contains(key)) { - updateFlagsNoLock(FLAG_ANNOTATED, true); - return; - } + for (String key: keySet()) { + if (getWorkInProgressKeys().contains(key)) { + updateFlagsNoLock(FLAG_ANNOTATED, true); + return; } } updateFlagsNoLock(FLAG_ANNOTATED, false); @@ -1190,9 +1186,13 @@ public abstract class OsmPrimitive extends AbstractPrimitive implements Comparab // We cannot directly use Arrays.equals(keys, other.keys) as keys is not ordered by key // but we can at least check if both arrays are null or of the same size before creating // and comparing the key maps (costly operation, see #7159) - return (keys == null && other.keys == null) - || (keys != null && other.keys != null && keys.length == other.keys.length - && (keys.length == 0 || getInterestingTags().equals(other.getInterestingTags()))); + // Note: This implementation is just broken. myKeys.length == 0 should not happen and + // myKeys.length == otherKeys.length is not required for the same interesting tags. + String[] myKeys = keys; + String[] otherKeys = other.keys; + return (myKeys == null && otherKeys == null) + || (myKeys != null && otherKeys != null && myKeys.length == otherKeys.length + && (myKeys.length == 0 || getInterestingTags().equals(other.getInterestingTags()))); } /** -- 1.9.1 From 23bb9c0e031d5407de2562f56eb7b339a4363c11 Mon Sep 17 00:00:00 2001 From: Michael Zangl Date: Sat, 2 Jan 2016 17:31:27 +0100 Subject: [PATCH 2/2] Fixed checkstyle. added links for RCU --- .../josm/data/osm/AbstractPrimitive.java | 24 +++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java b/src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java index c716f23..90ba106 100644 --- a/src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java +++ b/src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java @@ -14,6 +14,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Objects; import java.util.Set; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicLong; import org.openstreetmap.josm.tools.LanguageInfo; @@ -487,7 +488,16 @@ public abstract class AbstractPrimitive implements IPrimitive { * Note that the keys field is synchronized using RCU. * Writes to it are not synchronized by this object, the writers have to synchronize writes themselves. *

- * In short this means that you should not rely on this variable being the same value when read again and your should always copy it on writes. + * In short this means that you should not rely on this variable being the same value when read again and your should always + * copy it on writes. + *

+ * Further reading: + *

*/ protected volatile String[] keys; @@ -532,7 +542,8 @@ public abstract class AbstractPrimitive implements IPrimitive { * Old key/value pairs are removed. * If keys is null, clears existing key/value pairs. *

- * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used from multiple threads. + * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used + * from multiple threads. * * @param keys the key/value pairs to set. If null, removes all existing key/value pairs. */ @@ -558,7 +569,8 @@ public abstract class AbstractPrimitive implements IPrimitive { * Set the given value to the given key. If key is null, does nothing. If value is null, * removes the key and behaves like {@link #remove(String)}. *

- * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used from multiple threads. + * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used + * from multiple threads. * * @param key The key, for which the value is to be set. Can be null or empty, does nothing in this case. * @param value The value for the key. If null, removes the respective key/value pair. @@ -616,7 +628,8 @@ public abstract class AbstractPrimitive implements IPrimitive { /** * Remove the given key from the list *

- * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used from multiple threads. + * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used + * from multiple threads. * * @param key the key to be removed. Ignored, if key is null. */ @@ -646,7 +659,8 @@ public abstract class AbstractPrimitive implements IPrimitive { /** * Removes all keys from this primitive. *

- * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used from multiple threads. + * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used + * from multiple threads. */ @Override public void removeAll() { -- 1.9.1