Ticket #12272: patch-fix-keys-synchronisation.patch

src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java

@@ -481 +481 @@
      * 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.
+     * <p>
+     * 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.
+     * <p>
+     * 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 +531 @@
      * Sets the keys of this primitives to the key/value pairs in <code>keys</code>.
      * Old key/value pairs are removed.
      * If <code>keys</code> is null, clears existing key/value pairs.
+     * <p>
+     * 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 +557 @@
     /**
      * 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)}.
+     * <p>
+     * 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 +576 @@
             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
+     * <p>
+     * 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 +645 @@
 
     /**
      * Removes all keys from this primitive.
+     * <p>
+     * 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 +710 @@
     }
 
     public final int getNumKeys() {
+        String[] keys = this.keys;
         return keys == null ? 0 : keys.length / 2;
     }
 
@@ -718 +749 @@
      * @return true, if his primitive has a tag with key <code>key</code>
      */
     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;
     }
 
     /**

src/org/openstreetmap/josm/data/osm/OsmPrimitive.java

@@ -775 +775 @@
 
     /**
      * 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<String, String> getInterestingTags() {
         Map<String, String> result = new HashMap<>();
@@ -832 +832 @@
     }
 
     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 +1186 @@
         // 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())));
     }
 
     /**

src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java

@@ -14 +14 @@
 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 +488 @@
      * 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.
      * <p>
-     * 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.
+     * <p>
+     * Further reading:
+     * <ul>
+     * <li> The {@link CopyOnWriteArrayList} class
+     * <li> http://stackoverflow.com/questions/2950871/how-can-copyonwritearraylist-be-thread-safe
+     * <li> https://en.wikipedia.org/wiki/Read-copy-update
+     * (mind that we have a Garbage collector, rcu_assign_pointer and rcu_dereference are ensured by the volatile keyword)
+     * </ul>
      */
     protected volatile String[] keys;
 
@@ -532 +542 @@
      * Old key/value pairs are removed.
      * If <code>keys</code> is null, clears existing key/value pairs.
      * <p>
-     * 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 +569 @@
      * 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)}.
      * <p>
-     * 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 +628 @@
     /**
      * Remove the given key from the list
      * <p>
-     * 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 +659 @@
     /**
      * Removes all keys from this primitive.
      * <p>
-     * 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() {