[7.x] Fix circuit breaker leak in MultiTerms aggregation (elastic#79362…

…) (elastic#79421) The MultiTermsAggregator creates a BytesKeyedBucketOrds that never gets closed and therefore it might leak the memory allocated into the circuit breaker.
ywangd · Oct 19, 2021 · 220e3cf · 220e3cf
1 parent 4e89777
commit 220e3cf
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 3 deletions.
diff --git a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java
@@ -477,7 +477,7 @@ protected <A extends InternalAggregation, C extends Aggregator> A searchAndReduc
             indexSettings,
             query,
             breakerService,
-            builder.bytesToPreallocate(),
+            randomBoolean() ? 0 : builder.bytesToPreallocate(),
             maxBucket,
             fieldTypes
         );

diff --git a/x-pack/plugin/analytics/build.gradle b/x-pack/plugin/analytics/build.gradle
@@ -1,4 +1,6 @@
 apply plugin: 'elasticsearch.internal-es-plugin'
+apply plugin: 'elasticsearch.internal-cluster-test'
+
 esplugin {
   name 'x-pack-analytics'
   description 'Elasticsearch Expanded Pack Plugin - Analytics'

diff --git a/...est/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsWithRequestBreakerIT.java b/...est/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsWithRequestBreakerIT.java
@@ -0,0 +1,94 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.xpack.analytics.multiterms;
+
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.ExceptionsHelper;
+import org.elasticsearch.action.index.IndexRequestBuilder;
+import org.elasticsearch.common.breaker.CircuitBreakingException;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService;
+import org.elasticsearch.plugins.Plugin;
+import org.elasticsearch.search.aggregations.support.MultiValuesSourceFieldConfig;
+import org.elasticsearch.test.ESIntegTestCase;
+import org.elasticsearch.xpack.analytics.AnalyticsPlugin;
+
+import java.util.Collection;
+import java.util.stream.IntStream;
+
+/**
+ * test forked from CardinalityWithRequestBreakerIT
+ */
+public class MultiTermsWithRequestBreakerIT extends ESIntegTestCase {
+
+    protected Collection<Class<? extends Plugin>> nodePlugins() {
+        return org.elasticsearch.core.List.of(AnalyticsPlugin.class);
+    }
+
+    /**
+     * Test that searches using multiterms aggregations returns all request breaker memory.
+     */
+    public void testRequestBreaker() throws Exception {
+        final String requestBreaker = randomIntBetween(1, 10000) + "kb";
+        logger.info("--> Using request breaker setting: {}", requestBreaker);
+
+        indexRandom(
+            true,
+            IntStream.range(0, randomIntBetween(10, 1000))
+                .mapToObj(
+                    i -> client().prepareIndex("test", "_doc")
+                        .setId("id_" + i)
+                        .setSource(org.elasticsearch.core.Map.of("field0", randomAlphaOfLength(5), "field1", randomAlphaOfLength(5)))
+                )
+                .toArray(IndexRequestBuilder[]::new)
+        );
+
+        client().admin()
+            .cluster()
+            .prepareUpdateSettings()
+            .setPersistentSettings(
+                Settings.builder().put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING.getKey(), requestBreaker)
+            )
+            .get();
+
+        try {
+            client().prepareSearch("test")
+                .addAggregation(
+                    new MultiTermsAggregationBuilder("xxx").terms(
+                        org.elasticsearch.core.List.of(
+                            new MultiValuesSourceFieldConfig.Builder().setFieldName("field0.keyword").build(),
+                            new MultiValuesSourceFieldConfig.Builder().setFieldName("field1.keyword").build()
+                        )
+                    )
+                )
+                .get();
+        } catch (ElasticsearchException e) {
+            if (ExceptionsHelper.unwrap(e, CircuitBreakingException.class) == null) {
+                throw e;
+            }
+        }
+
+        client().admin()
+            .cluster()
+            .prepareUpdateSettings()
+            .setPersistentSettings(
+                Settings.builder().putNull(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING.getKey())
+            )
+            .get();
+
+        // validation done by InternalTestCluster.ensureEstimatedStats()
+    }
+}
diff --git a/...tics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsAggregator.java b/...tics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsAggregator.java
@@ -19,6 +19,7 @@
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.core.CheckedConsumer;
+import org.elasticsearch.core.Releasables;
 import org.elasticsearch.index.fielddata.SortedBinaryDocValues;
 import org.elasticsearch.index.fielddata.SortedNumericDoubleValues;
 import org.elasticsearch.search.DocValueFormat;
@@ -110,7 +111,6 @@ protected MultiTermsAggregator(
             .collect(Collectors.toList());
         keyConverters = values.stream().map(TermValuesSource::keyConverter).collect(Collectors.toList());
         bucketOrds = BytesKeyedBucketOrds.build(context.bigArrays(), cardinality);
-
     }
 
     private boolean subAggsNeedScore() {
@@ -220,6 +220,11 @@ public void accept(Integer start) throws IOException {
         };
     }
 
+    @Override
+    protected void doClose() {
+        Releasables.close(bucketOrds);
+    }
+
     @Override
     public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws IOException {
         InternalMultiTerms.Bucket[][] topBucketsPerOrd = new InternalMultiTerms.Bucket[owningBucketOrds.length][];

diff --git a/...rc/main/java/org/elasticsearch/xpack/ml/aggs/categorization/CategorizeTextAggregator.java b/...rc/main/java/org/elasticsearch/xpack/ml/aggs/categorization/CategorizeTextAggregator.java
@@ -105,7 +105,7 @@ protected CategorizeTextAggregator(
     @Override
     protected void doClose() {
         super.doClose();
-        Releasables.close(this.analyzer, this.bytesRefHash);
+        Releasables.close(this.analyzer, this.bytesRefHash, this.bucketOrds, this.categorizers);
     }
 
     @Override