diff --git a/lucene/core/src/java/org/apache/lucene/index/FieldInfo.java b/lucene/core/src/java/org/apache/lucene/index/FieldInfo.java index 7c2afd4177d0..4b80441f2a46 100644 --- a/lucene/core/src/java/org/apache/lucene/index/FieldInfo.java +++ b/lucene/core/src/java/org/apache/lucene/index/FieldInfo.java @@ -16,6 +16,8 @@ */ package org.apache.lucene.index; +import java.util.Collections; +import java.util.HashMap; import java.util.Map; import java.util.Objects; @@ -42,7 +44,7 @@ public final class FieldInfo { private final IndexOptions indexOptions; private boolean storePayloads; // whether this field stores payloads together with term positions - private final Map attributes; + private Map attributes; private long dvGen; @@ -782,7 +784,7 @@ public boolean hasVectorValues() { } /** Get a codec attribute value, or null if it does not exist */ - public String getAttribute(String key) { + public synchronized String getAttribute(String key) { return attributes.get(key); } @@ -797,12 +799,17 @@ public String getAttribute(String key) { * If the value of the attributes for a same field is changed between the documents, the behaviour * after merge is undefined. */ - public String putAttribute(String key, String value) { - return attributes.put(key, value); + public synchronized String putAttribute(String key, String value) { + HashMap newMap = new HashMap<>(attributes); + String oldValue = newMap.put(key, value); + // This needs to be thread-safe as multiple threads may be updating (different) attributes + // concurrently due to concurrent merging. + attributes = Collections.unmodifiableMap(newMap); + return oldValue; } /** Returns internal codec attributes map. */ - public Map attributes() { + public synchronized Map attributes() { return attributes; } diff --git a/lucene/core/src/java/org/apache/lucene/index/SegmentInfo.java b/lucene/core/src/java/org/apache/lucene/index/SegmentInfo.java index b28197023152..74528f976721 100644 --- a/lucene/core/src/java/org/apache/lucene/index/SegmentInfo.java +++ b/lucene/core/src/java/org/apache/lucene/index/SegmentInfo.java @@ -249,6 +249,7 @@ public String toString(int delCount) { s.append(']'); } + Map attributes = getAttributes(); if (!attributes.isEmpty()) { s.append(":[attributes="); s.append(attributes.toString()); @@ -342,7 +343,7 @@ String namedForThisSegment(String file) { } /** Get a codec attribute value, or null if it does not exist */ - public String getAttribute(String key) { + public synchronized String getAttribute(String key) { return attributes.get(key); } @@ -356,17 +357,12 @@ public String getAttribute(String key) { *

If a value already exists for the field, it will be replaced with the new value. This method * make a copy on write for every attribute change. */ - public String putAttribute(String key, String value) { + public synchronized String putAttribute(String key, String value) { HashMap newMap = new HashMap<>(attributes); String oldValue = newMap.put(key, value); - // we make a full copy of this to prevent concurrent modifications to this in the toString - // method - // this method is only called when a segment is written but the SegmentInfo might be exposed - // in running merges which can cause ConcurrentModificationExceptions if we modify / share - // the same instance. Technically that's an unsafe publication but IW design would require - // significant changes to prevent this. On the other hand, since we expose the map in - // getAttributes() - // it's a good design to make it unmodifiable anyway. + // This needs to be thread-safe because multiple threads may be updating (different) attributes + // at the same time due to concurrent merging, plus some threads may be calling toString() on + // segment info while other threads are updating attributes. attributes = Collections.unmodifiableMap(newMap); return oldValue; } @@ -376,7 +372,7 @@ public String putAttribute(String key, String value) { * * @return internal codec attributes map. */ - public Map getAttributes() { + public synchronized Map getAttributes() { return attributes; }