Changeset 9267 in josm for trunk/src/org/openstreetmap/josm


Ignore:
Timestamp:
2016-01-02T19:15:51+01:00 (8 years ago)
Author:
simon04
Message:

fix #12272 - Fix access to the AbstractPrimitive#keys (patch by michael2402)

Location:
trunk/src/org/openstreetmap/josm/data/osm
Files:
2 edited

Legend:

Unmodified
Added
Removed

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

    r9243 r9267  
    482482     ------------*/
    483483
    484     // Note that all methods that read keys first make local copy of keys array reference. This is to ensure thread safety - reading
    485     // 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
    486     // the array itself will be never modified - only reference will be changed
    487 
    488484    /**
    489485     * The key/value list for this primitive.
    490      */
    491     protected String[] keys;
     486     * <p>
     487     * Note that the keys field is synchronized using RCU.
     488     * Writes to it are not synchronized by this object, the writers have to synchronize writes themselves.
     489     * <p>
     490     * In short this means that you should not rely on this variable being the same value when read again and your should always
     491     * copy it on writes.
     492     * <p>
     493     * Further reading:
     494     * <ul>
     495     * <li>{@link java.util.concurrent.CopyOnWriteArrayList}</li>
     496     * <li> <a href="http://stackoverflow.com/questions/2950871/how-can-copyonwritearraylist-be-thread-safe">
     497     *     http://stackoverflow.com/questions/2950871/how-can-copyonwritearraylist-be-thread-safe</a></li>
     498     * <li> <a href="https://en.wikipedia.org/wiki/Read-copy-update">
     499     *     https://en.wikipedia.org/wiki/Read-copy-update</a> (mind that we have a Garbage collector,
     500     *     {@code rcu_assign_pointer} and {@code rcu_dereference} are ensured by the {@code volatile} keyword)</li>
     501     * </ul>
     502     */
     503    protected volatile String[] keys;
    492504
    493505    /**
    … …  
    531543     * Old key/value pairs are removed.
    532544     * If <code>keys</code> is null, clears existing key/value pairs.
     545     * <p>
     546     * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used
     547     * from multiple threads.
    533548     *
    534549     * @param keys the key/value pairs to set. If null, removes all existing key/value pairs.
    … …  
    555570     * Set the given value to the given key. If key is null, does nothing. If value is null,
    556571     * removes the key and behaves like {@link #remove(String)}.
     572     * <p>
     573     * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used
     574     * from multiple threads.
    557575     *
    558576     * @param key  The key, for which the value is to be set. Can be null or empty, does nothing in this case.
    … …  
    572590            keysChangedImpl(originalKeys);
    573591        } else {
    574             for (int i = 0; i < keys.length; i += 2) {
    575                 if (keys[i].equals(key)) {
    576                     // This modifies the keys array but it doesn't make it invalidate for any time so its ok (see note no top)
    577                     keys[i+1] = value;
    578                     keysChangedImpl(originalKeys);
    579                     return;
    580                 }
     592            int keyIndex = indexOfKey(keys, key);
     593            int tagArrayLength = keys.length;
     594            if (keyIndex < 0) {
     595                keyIndex = tagArrayLength;
     596                tagArrayLength += 2;
    581597            }
    582             String[] newKeys = Arrays.copyOf(keys, keys.length + 2);
    583             newKeys[keys.length] = key;
    584             newKeys[keys.length + 1] = value;
     598
     599            // Do not try to optimize this array creation if the key already exists.
     600            // We would need to convert the keys array to be an AtomicReferenceArray
     601            // Or we would at least need a volatile write after the array was modified to
     602            // ensure that changes are visible by other threads.
     603            String[] newKeys = Arrays.copyOf(keys, tagArrayLength);
     604            newKeys[keyIndex] = key;
     605            newKeys[keyIndex + 1] = value;
    585606            keys = newKeys;
    586607            keysChangedImpl(originalKeys);
    … …  
    589610
    590611    /**
     612     * Scans a key/value array for a given key.
     613     * @param keys The key array. It is not modified. It may be null to indicate an emtpy array.
     614     * @param key The key to search for.
     615     * @return The position of that key in the keys array - which is always a multiple of 2 - or -1 if it was not found.
     616     */
     617    private static int indexOfKey(String[] keys, String key) {
     618        if (keys == null) {
     619            return -1;
     620        }
     621        for (int i = 0; i < keys.length; i += 2) {
     622            if (keys[i].equals(key)) {
     623                return i;
     624            }
     625        }
     626        return -1;
     627    }
     628
     629    /**
    591630     * Remove the given key from the list
     631     * <p>
     632     * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used
     633     * from multiple threads.
    592634     *
    593635     * @param key  the key to be removed. Ignored, if key is null.
    … …  
    618660    /**
    619661     * Removes all keys from this primitive.
     662     * <p>
     663     * Note that this method, like all methods that modify keys, is not synchronized and may lead to data corruption when being used
     664     * from multiple threads.
    620665     */
    621666    @Override
    … …  
    681726
    682727    public final int getNumKeys() {
     728        String[] keys = this.keys;
    683729        return keys == null ? 0 : keys.length / 2;
    684730    }
    … …  
    719765     */
    720766    public boolean hasKey(String key) {
    721         String[] keys = this.keys;
    722         if (key == null) return false;
    723         if (keys == null) return false;
    724         for (int i = 0; i < keys.length; i += 2) {
    725             if (keys[i].equals(key)) return true;
    726         }
    727         return false;
     767        return key != null && indexOfKey(keys, key) >= 0;
    728768    }
    729769

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

    r9243 r9267  
    776776    /**
    777777     * Returns {@link #getKeys()} for which {@code key} does not fulfill {@link #isUninterestingKey}.
    778      * @return list of interesting tags
     778     * @return A map of interesting tags
    779779     */
    780780    public Map<String, String> getInterestingTags() {
    … …  
    833833
    834834    private void updateTagged() {
    835         if (keys != null) {
    836             for (String key: keySet()) {
    837                 // 'area' is not really uninteresting (putting it in that list may have unpredictable side effects)
    838                 // but it's clearly not enough to consider an object as tagged (see #9261)
    839                 if (!isUninterestingKey(key) && !"area".equals(key)) {
    840                     updateFlagsNoLock(FLAG_TAGGED, true);
    841                     return;
    842                 }
     835        for (String key: keySet()) {
     836            // 'area' is not really uninteresting (putting it in that list may have unpredictable side effects)
     837            // but it's clearly not enough to consider an object as tagged (see #9261)
     838            if (!isUninterestingKey(key) && !"area".equals(key)) {
     839                updateFlagsNoLock(FLAG_TAGGED, true);
     840                return;
    843841            }
    844842        }
    … …  
    847845
    848846    private void updateAnnotated() {
    849         if (keys != null) {
    850             for (String key: keySet()) {
    851                 if (getWorkInProgressKeys().contains(key)) {
    852                     updateFlagsNoLock(FLAG_ANNOTATED, true);
    853                     return;
    854                 }
     847        for (String key: keySet()) {
     848            if (getWorkInProgressKeys().contains(key)) {
     849                updateFlagsNoLock(FLAG_ANNOTATED, true);
     850                return;
    855851            }
    856852        }
    … …  
    11881184     */
    11891185    public boolean hasSameInterestingTags(OsmPrimitive other) {
    1190         // We cannot directly use Arrays.equals(keys, other.keys) as keys is not ordered by key
    1191         // but we can at least check if both arrays are null or of the same size before creating
    1192         // and comparing the key maps (costly operation, see #7159)
    11931186        return (keys == null && other.keys == null)
    1194                 || (keys != null && other.keys != null && keys.length == other.keys.length
    1195                         && (keys.length == 0 || getInterestingTags().equals(other.getInterestingTags())));
     1187                || getInterestingTags().equals(other.getInterestingTags());
    11961188    }
    11971189
Note: See TracChangeset for help on using the changeset viewer.