Make segment/field attribute updates thread-safe. (#13331)

Because of concurrent merging (#13124), multiple threads may be updating
(different) attributes concurrently, so we need to make reads and writes to
attributes thread-safe.

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<String, String> attributes;
+  private Map<String, String> 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<String, String> 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<String, String> attributes() {
+  public synchronized Map<String, String> attributes() {
     return attributes;
   }
 

lucene/core/src/java/org/apache/lucene/index/SegmentInfo.java

@@ -249,6 +249,7 @@ public String toString(int delCount) {
       s.append(']');
     }
 
+    Map<String, String> 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) {
    * <p>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<String, String> 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<String, String> getAttributes() {
+  public synchronized Map<String, String> getAttributes() {
     return attributes;
   }