don't wrap parentTypes or fullPathObjectMappers unless the references has changed

Author: jkiang13

core/src/main/java/org/elasticsearch/index/mapper/MapperService.java

@@ -335,7 +335,7 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
         boolean allEnabled = this.allEnabled;
         Map<String, ObjectMapper> fullPathObjectMappers = this.fullPathObjectMappers;
         FieldTypeLookup fieldTypes = this.fieldTypes;
-        Set<String> parentTypes = new HashSet<>(this.parentTypes);
+        Set<String> parentTypes = this.parentTypes;
         Map<String, DocumentMapper> mappers = new HashMap<>(this.mappers);
 
         Map<String, DocumentMapper> results = new LinkedHashMap<>(documentMappers.size() + 1);
@@ -394,6 +394,7 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
 
             for (ObjectMapper objectMapper : objectMappers) {
                 if (fullPathObjectMappers == this.fullPathObjectMappers) {
+                    // first time through the loops
                     fullPathObjectMappers = new HashMap<>(this.fullPathObjectMappers);
                 }
                 fullPathObjectMappers.put(objectMapper.fullPath(), objectMapper);
@@ -413,6 +414,10 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
             }
 
             if (oldMapper == null && newMapper.parentFieldMapper().active()) {
+                if (parentTypes == this.parentTypes) {
+                    // first time through the loop
+                    parentTypes = new HashSet<>(this.parentTypes);
+                }
                 parentTypes.add(mapper.parentFieldMapper().type());
             }
 
@@ -453,8 +458,15 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
         // make structures immutable
         mappers = Collections.unmodifiableMap(mappers);
         results = Collections.unmodifiableMap(results);
-        parentTypes = Collections.unmodifiableSet(parentTypes);
-        fullPathObjectMappers = Collections.unmodifiableMap(fullPathObjectMappers);
+
+        // only need to immutably rewrap these if the previous reference was changed.
+        // if not then they are already implicitly immutable.
+        if (fullPathObjectMappers != this.fullPathObjectMappers) {
+            fullPathObjectMappers = Collections.unmodifiableMap(fullPathObjectMappers);
+        }
+        if (parentTypes != this.parentTypes) {
+            parentTypes = Collections.unmodifiableSet(parentTypes);
+        }
 
         // commit the change
         if (defaultMappingSource != null) {

core/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java

@@ -38,6 +38,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.function.Function;
 
@@ -189,6 +190,22 @@ public void testMergeWithMap() throws Throwable {
         assertThat(e.getMessage(), startsWith("Failed to parse mapping [type1]: "));
     }
 
+    public void testMergeParentTypesSame() {
+        // Verifies that a merge (absent a DocumentMapper change)
+        // doesn't change the parentTypes reference.
+        // The collection was being rewrapped with each merge
+        // in v5.2 resulting in eventual StackOverflowErrors.
+        // https://github.com/elastic/elasticsearch/issues/23604
+
+        IndexService indexService1 = createIndex("index1");
+        MapperService mapperService = indexService1.mapperService();
+        Set<String> parentTypes = mapperService.getParentTypes();
+
+        Map<String, Map<String, Object>> mappings = new HashMap<>();
+        mapperService.merge(mappings, MergeReason.MAPPING_UPDATE, false);
+        assertSame(parentTypes, mapperService.getParentTypes());
+    }
+
     public void testOtherDocumentMappersOnlyUpdatedWhenChangingFieldType() throws IOException {
         IndexService indexService = createIndex("test");