Fix MapperService StackOverflowError (#23605)
MapperService#parentTypes is rewrapped in an UnmodifiableSet in MapperService#internalMerge every time the cluster state is updated. After thousands of updates the collection is wrapped so deeply that calling a method on it generates a StackOverflowError. Closes #23604
This commit is contained in:
parent
2eafe8310e
commit
d010cad503
|
@ -110,7 +110,7 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
|
||||||
private volatile Map<String, DocumentMapper> mappers = emptyMap();
|
private volatile Map<String, DocumentMapper> mappers = emptyMap();
|
||||||
|
|
||||||
private volatile FieldTypeLookup fieldTypes;
|
private volatile FieldTypeLookup fieldTypes;
|
||||||
private volatile Map<String, ObjectMapper> fullPathObjectMappers = new HashMap<>();
|
private volatile Map<String, ObjectMapper> fullPathObjectMappers = emptyMap();
|
||||||
private boolean hasNested = false; // updated dynamically to true when a nested object is added
|
private boolean hasNested = false; // updated dynamically to true when a nested object is added
|
||||||
private boolean allEnabled = false; // updated dynamically to true when _all is enabled
|
private boolean allEnabled = false; // updated dynamically to true when _all is enabled
|
||||||
|
|
||||||
|
@ -394,6 +394,7 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
|
||||||
|
|
||||||
for (ObjectMapper objectMapper : objectMappers) {
|
for (ObjectMapper objectMapper : objectMappers) {
|
||||||
if (fullPathObjectMappers == this.fullPathObjectMappers) {
|
if (fullPathObjectMappers == this.fullPathObjectMappers) {
|
||||||
|
// first time through the loops
|
||||||
fullPathObjectMappers = new HashMap<>(this.fullPathObjectMappers);
|
fullPathObjectMappers = new HashMap<>(this.fullPathObjectMappers);
|
||||||
}
|
}
|
||||||
fullPathObjectMappers.put(objectMapper.fullPath(), objectMapper);
|
fullPathObjectMappers.put(objectMapper.fullPath(), objectMapper);
|
||||||
|
@ -414,6 +415,7 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
|
||||||
|
|
||||||
if (oldMapper == null && newMapper.parentFieldMapper().active()) {
|
if (oldMapper == null && newMapper.parentFieldMapper().active()) {
|
||||||
if (parentTypes == this.parentTypes) {
|
if (parentTypes == this.parentTypes) {
|
||||||
|
// first time through the loop
|
||||||
parentTypes = new HashSet<>(this.parentTypes);
|
parentTypes = new HashSet<>(this.parentTypes);
|
||||||
}
|
}
|
||||||
parentTypes.add(mapper.parentFieldMapper().type());
|
parentTypes.add(mapper.parentFieldMapper().type());
|
||||||
|
@ -456,8 +458,15 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
|
||||||
// make structures immutable
|
// make structures immutable
|
||||||
mappers = Collections.unmodifiableMap(mappers);
|
mappers = Collections.unmodifiableMap(mappers);
|
||||||
results = Collections.unmodifiableMap(results);
|
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
|
// commit the change
|
||||||
if (defaultMappingSource != null) {
|
if (defaultMappingSource != null) {
|
||||||
|
|
|
@ -38,6 +38,7 @@ import java.util.Collections;
|
||||||
import java.util.HashMap;
|
import java.util.HashMap;
|
||||||
import java.util.HashSet;
|
import java.util.HashSet;
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
|
import java.util.Set;
|
||||||
import java.util.concurrent.ExecutionException;
|
import java.util.concurrent.ExecutionException;
|
||||||
import java.util.function.Function;
|
import java.util.function.Function;
|
||||||
|
|
||||||
|
@ -189,6 +190,22 @@ public class MapperServiceTests extends ESSingleNodeTestCase {
|
||||||
assertThat(e.getMessage(), startsWith("Failed to parse mapping [type1]: "));
|
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 {
|
public void testOtherDocumentMappersOnlyUpdatedWhenChangingFieldType() throws IOException {
|
||||||
IndexService indexService = createIndex("test");
|
IndexService indexService = createIndex("test");
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue