Automatically close releasables after test (#24687)

This moves the releasing logic to the base test, so that individual test cases don't need
to worry about releasing the aggregators.  It's not a big deal for individual aggs,
but once tests start using sub-aggs, it can become tricky to free (without double-freeing)
all the aggregators.

core/src/test/java/org/elasticsearch/search/aggregations/bucket/GlobalAggregatorTests.java

@@ -78,17 +78,14 @@ public class GlobalAggregatorTests extends AggregatorTestCase {
         aggregationBuilder.subAggregation(new MinAggregationBuilder("in_global").field("number"));
         MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
         fieldType.setName("number");
-        try (GlobalAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-            try {
-                aggregator.preCollection();
-                indexSearcher.search(new MatchAllDocsQuery(), aggregator);
-                aggregator.postCollection();
-                InternalGlobal result = (InternalGlobal) aggregator.buildAggregation(0L);
-                verify.accept(result, (InternalMin) result.getAggregations().asMap().get("in_global"));
-            } finally {
-                IOUtils.close(aggregator.subAggregators());
-            }
-        }
+
+        GlobalAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+        aggregator.preCollection();
+        indexSearcher.search(new MatchAllDocsQuery(), aggregator);
+        aggregator.postCollection();
+        InternalGlobal result = (InternalGlobal) aggregator.buildAggregation(0L);
+        verify.accept(result, (InternalMin) result.getAggregations().asMap().get("in_global"));
+
         indexReader.close();
         directory.close();
     }

core/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java

@@ -112,12 +112,13 @@ public class GeoHashGridAggregatorTests extends AggregatorTestCase {
         MappedFieldType fieldType = new GeoPointFieldMapper.GeoPointFieldType();
         fieldType.setHasDocValues(true);
         fieldType.setName(FIELD_NAME);
-        try (Aggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-            aggregator.preCollection();
-            indexSearcher.search(query, aggregator);
-            aggregator.postCollection();
-            verify.accept((InternalGeoHashGrid) aggregator.buildAggregation(0L));
-        }
+
+        Aggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+        aggregator.preCollection();
+        indexSearcher.search(query, aggregator);
+        aggregator.postCollection();
+        verify.accept((InternalGeoHashGrid) aggregator.buildAggregation(0L));
+
         indexReader.close();
         directory.close();
     }

core/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java

@@ -75,21 +75,22 @@ public class TermsAggregatorTests extends AggregatorTestCase {
             MappedFieldType fieldType = new KeywordFieldMapper.KeywordFieldType();
             fieldType.setName("string");
             fieldType.setHasDocValues(true );
-            try (TermsAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-                aggregator.preCollection();
-                indexSearcher.search(new MatchAllDocsQuery(), aggregator);
-                aggregator.postCollection();
-                Terms result = (Terms) aggregator.buildAggregation(0L);
-                assertEquals(4, result.getBuckets().size());
-                assertEquals("a", result.getBuckets().get(0).getKeyAsString());
-                assertEquals(2L, result.getBuckets().get(0).getDocCount());
-                assertEquals("b", result.getBuckets().get(1).getKeyAsString());
-                assertEquals(2L, result.getBuckets().get(1).getDocCount());
-                assertEquals("c", result.getBuckets().get(2).getKeyAsString());
-                assertEquals(1L, result.getBuckets().get(2).getDocCount());
-                assertEquals("d", result.getBuckets().get(3).getKeyAsString());
-                assertEquals(1L, result.getBuckets().get(3).getDocCount());
-            }
+
+            TermsAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+            aggregator.preCollection();
+            indexSearcher.search(new MatchAllDocsQuery(), aggregator);
+            aggregator.postCollection();
+            Terms result = (Terms) aggregator.buildAggregation(0L);
+            assertEquals(4, result.getBuckets().size());
+            assertEquals("a", result.getBuckets().get(0).getKeyAsString());
+            assertEquals(2L, result.getBuckets().get(0).getDocCount());
+            assertEquals("b", result.getBuckets().get(1).getKeyAsString());
+            assertEquals(2L, result.getBuckets().get(1).getDocCount());
+            assertEquals("c", result.getBuckets().get(2).getKeyAsString());
+            assertEquals(1L, result.getBuckets().get(2).getDocCount());
+            assertEquals("d", result.getBuckets().get(3).getKeyAsString());
+            assertEquals(1L, result.getBuckets().get(3).getDocCount());
+
         }
         indexReader.close();
         directory.close();
@@ -191,12 +192,11 @@ public class TermsAggregatorTests extends AggregatorTestCase {
 
     private InternalAggregation buildInternalAggregation(TermsAggregationBuilder builder, MappedFieldType fieldType,
                                                          IndexSearcher searcher) throws IOException {
-        try (TermsAggregator aggregator = createAggregator(builder, searcher, fieldType)) {
-            aggregator.preCollection();
-            searcher.search(new MatchAllDocsQuery(), aggregator);
-            aggregator.postCollection();
-            return aggregator.buildAggregation(0L);
-        }
+        TermsAggregator aggregator = createAggregator(builder, searcher, fieldType);
+        aggregator.preCollection();
+        searcher.search(new MatchAllDocsQuery(), aggregator);
+        aggregator.postCollection();
+        return aggregator.buildAggregation(0L);
     }
 
 }

core/src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java

@@ -118,13 +118,13 @@ public class CardinalityAggregatorTests extends AggregatorTestCase {
         MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(
                 NumberFieldMapper.NumberType.LONG);
         fieldType.setName("number");
-        try (CardinalityAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher,
-                fieldType)) {
-            aggregator.preCollection();
-            indexSearcher.search(query, aggregator);
-            aggregator.postCollection();
-            verify.accept((InternalCardinality) aggregator.buildAggregation(0L));
-        }
+        CardinalityAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher,
+            fieldType);
+        aggregator.preCollection();
+        indexSearcher.search(query, aggregator);
+        aggregator.postCollection();
+        verify.accept((InternalCardinality) aggregator.buildAggregation(0L));
+
         indexReader.close();
         directory.close();
     }

core/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxAggregatorTests.java

@@ -112,12 +112,13 @@ public class MaxAggregatorTests extends AggregatorTestCase {
         MaxAggregationBuilder aggregationBuilder = new MaxAggregationBuilder("_name").field("number");
         MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
         fieldType.setName("number");
-        try (MaxAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-            aggregator.preCollection();
-            indexSearcher.search(query, aggregator);
-            aggregator.postCollection();
-            verify.accept((InternalMax) aggregator.buildAggregation(0L));
-        }
+
+        MaxAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+        aggregator.preCollection();
+        indexSearcher.search(query, aggregator);
+        aggregator.postCollection();
+        verify.accept((InternalMax) aggregator.buildAggregation(0L));
+
         indexReader.close();
         directory.close();
     }

core/src/test/java/org/elasticsearch/search/aggregations/metrics/avg/AvgAggregatorTests.java

@@ -113,12 +113,13 @@ public class AvgAggregatorTests extends AggregatorTestCase {
         AvgAggregationBuilder aggregationBuilder = new AvgAggregationBuilder("_name").field("number");
         MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
         fieldType.setName("number");
-        try (AvgAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-            aggregator.preCollection();
-            indexSearcher.search(query, aggregator);
-            aggregator.postCollection();
-            verify.accept((InternalAvg) aggregator.buildAggregation(0L));
-        }
+
+        AvgAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+        aggregator.preCollection();
+        indexSearcher.search(query, aggregator);
+        aggregator.postCollection();
+        verify.accept((InternalAvg) aggregator.buildAggregation(0L));
+
         indexReader.close();
         directory.close();
     }

core/src/test/java/org/elasticsearch/search/aggregations/metrics/min/MinAggregatorTests.java

@@ -62,13 +62,14 @@ public class MinAggregatorTests extends AggregatorTestCase {
         MinAggregationBuilder aggregationBuilder = new MinAggregationBuilder("_name").field("number");
         MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
         fieldType.setName("number");
-        try (MinAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-            aggregator.preCollection();
-            indexSearcher.search(new MatchAllDocsQuery(), aggregator);
-            aggregator.postCollection();
-            InternalMin result = (InternalMin) aggregator.buildAggregation(0L);
-            assertEquals(-1.0, result.getValue(), 0);
-        }
+
+        MinAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+        aggregator.preCollection();
+        indexSearcher.search(new MatchAllDocsQuery(), aggregator);
+        aggregator.postCollection();
+        InternalMin result = (InternalMin) aggregator.buildAggregation(0L);
+        assertEquals(-1.0, result.getValue(), 0);
+
         indexReader.close();
         directory.close();
     }
@@ -96,13 +97,14 @@ public class MinAggregatorTests extends AggregatorTestCase {
         MinAggregationBuilder aggregationBuilder = new MinAggregationBuilder("_name").field("number");
         MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
         fieldType.setName("number");
-        try (MinAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-            aggregator.preCollection();
-            indexSearcher.search(new MatchAllDocsQuery(), aggregator);
-            aggregator.postCollection();
-            InternalMin result = (InternalMin) aggregator.buildAggregation(0L);
-            assertEquals(-1.0, result.getValue(), 0);
-        }
+
+        MinAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+        aggregator.preCollection();
+        indexSearcher.search(new MatchAllDocsQuery(), aggregator);
+        aggregator.postCollection();
+        InternalMin result = (InternalMin) aggregator.buildAggregation(0L);
+        assertEquals(-1.0, result.getValue(), 0);
+
         indexReader.close();
         directory.close();
     }
@@ -127,13 +129,14 @@ public class MinAggregatorTests extends AggregatorTestCase {
         MinAggregationBuilder aggregationBuilder = new MinAggregationBuilder("_name").field("number2");
         MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
         fieldType.setName("number2");
-        try (MinAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-            aggregator.preCollection();
-            indexSearcher.search(new MatchAllDocsQuery(), aggregator);
-            aggregator.postCollection();
-            InternalMin result = (InternalMin) aggregator.buildAggregation(0L);
-            assertEquals(Double.POSITIVE_INFINITY, result.getValue(), 0);
-        }
+
+        MinAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+        aggregator.preCollection();
+        indexSearcher.search(new MatchAllDocsQuery(), aggregator);
+        aggregator.postCollection();
+        InternalMin result = (InternalMin) aggregator.buildAggregation(0L);
+        assertEquals(Double.POSITIVE_INFINITY, result.getValue(), 0);
+
         indexReader.close();
         directory.close();
     }
@@ -149,13 +152,14 @@ public class MinAggregatorTests extends AggregatorTestCase {
         MinAggregationBuilder aggregationBuilder = new MinAggregationBuilder("_name").field("number");
         MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
         fieldType.setName("number");
-        try (MinAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-            aggregator.preCollection();
-            indexSearcher.search(new MatchAllDocsQuery(), aggregator);
-            aggregator.postCollection();
-            InternalMin result = (InternalMin) aggregator.buildAggregation(0L);
-            assertEquals(Double.POSITIVE_INFINITY, result.getValue(), 0);
-        }
+
+        MinAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+        aggregator.preCollection();
+        indexSearcher.search(new MatchAllDocsQuery(), aggregator);
+        aggregator.postCollection();
+        InternalMin result = (InternalMin) aggregator.buildAggregation(0L);
+        assertEquals(Double.POSITIVE_INFINITY, result.getValue(), 0);
+
         indexReader.close();
         directory.close();
     }

core/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/HDRPercentilesAggregatorTests.java

@@ -127,12 +127,12 @@ public class HDRPercentilesAggregatorTests extends AggregatorTestCase {
 
                 MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
                 fieldType.setName("number");
-                try (HDRPercentilesAggregator aggregator = createAggregator(builder, indexSearcher, fieldType)) {
-                    aggregator.preCollection();
-                    indexSearcher.search(query, aggregator);
-                    aggregator.postCollection();
-                    verify.accept((InternalHDRPercentiles) aggregator.buildAggregation(0L));
-                }
+                HDRPercentilesAggregator aggregator = createAggregator(builder, indexSearcher, fieldType);
+                aggregator.preCollection();
+                indexSearcher.search(query, aggregator);
+                aggregator.postCollection();
+                verify.accept((InternalHDRPercentiles) aggregator.buildAggregation(0L));
+
             }
         }
     }

core/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/TDigestPercentilesAggregatorTests.java

@@ -148,12 +148,11 @@ public class TDigestPercentilesAggregatorTests extends AggregatorTestCase {
 
                 MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
                 fieldType.setName("number");
-                try (TDigestPercentilesAggregator aggregator = createAggregator(builder, indexSearcher, fieldType)) {
-                    aggregator.preCollection();
-                    indexSearcher.search(query, aggregator);
-                    aggregator.postCollection();
-                    verify.accept((InternalTDigestPercentiles) aggregator.buildAggregation(0L));
-                }
+                TDigestPercentilesAggregator aggregator = createAggregator(builder, indexSearcher, fieldType);
+                aggregator.preCollection();
+                indexSearcher.search(query, aggregator);
+                aggregator.postCollection();
+                verify.accept((InternalTDigestPercentiles) aggregator.buildAggregation(0L));
             }
         }
     }

core/src/test/java/org/elasticsearch/search/aggregations/metrics/sum/SumAggregatorTests.java

@@ -132,13 +132,12 @@ public class SumAggregatorTests extends AggregatorTestCase {
                 SumAggregationBuilder aggregationBuilder = new SumAggregationBuilder("_name");
                 aggregationBuilder.field(FIELD_NAME);
 
-                try (SumAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-                    aggregator.preCollection();
-                    indexSearcher.search(query, aggregator);
-                    aggregator.postCollection();
+                SumAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+                aggregator.preCollection();
+                indexSearcher.search(query, aggregator);
+                aggregator.postCollection();
 
-                    verify.accept((Sum) aggregator.buildAggregation(0L));
-                }
+                verify.accept((Sum) aggregator.buildAggregation(0L));
             }
         }
     }

core/src/test/java/org/elasticsearch/search/aggregations/metrics/valuecount/ValueCountAggregatorTests.java

@@ -120,13 +120,11 @@ public class ValueCountAggregatorTests extends AggregatorTestCase {
                 ValueCountAggregationBuilder aggregationBuilder = new ValueCountAggregationBuilder("_name", valueType);
                 aggregationBuilder.field(FIELD_NAME);
 
-                try (ValueCountAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType)) {
-                    aggregator.preCollection();
-                    indexSearcher.search(query, aggregator);
-                    aggregator.postCollection();
-
-                    verify.accept((ValueCount) aggregator.buildAggregation(0L));
-                }
+                ValueCountAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
+                aggregator.preCollection();
+                indexSearcher.search(query, aggregator);
+                aggregator.postCollection();
+                verify.accept((ValueCount) aggregator.buildAggregation(0L));
             }
         }
     }

test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java

@@ -63,6 +63,7 @@ import org.elasticsearch.search.internal.ContextIndexSearcher;
 import org.elasticsearch.search.internal.SearchContext;
 import org.elasticsearch.search.lookup.SearchLookup;
 import org.elasticsearch.test.ESTestCase;
+import org.junit.After;
 import org.mockito.Matchers;
 
 import java.io.IOException;
@@ -203,17 +204,13 @@ public abstract class AggregatorTestCase extends ESTestCase {
                                                                              AggregationBuilder builder,
                                                                              MappedFieldType... fieldTypes) throws IOException {
         C a = createAggregator(builder, searcher, fieldTypes);
-        try {
-            a.preCollection();
-            searcher.search(query, a);
-            a.postCollection();
-            @SuppressWarnings("unchecked")
-            A internalAgg = (A) a.buildAggregation(0L);
-            return internalAgg;
-        } finally {
-            Releasables.close(releasables);
-            releasables.clear();
-        }
+        a.preCollection();
+        searcher.search(query, a);
+        a.postCollection();
+        @SuppressWarnings("unchecked")
+        A internalAgg = (A) a.buildAggregation(0L);
+        return internalAgg;
+
     }
 
     /**
@@ -245,38 +242,35 @@ public abstract class AggregatorTestCase extends ESTestCase {
         Query rewritten = searcher.rewrite(query);
         Weight weight = searcher.createWeight(rewritten, true, 1f);
         C root = createAggregator(builder, searcher, fieldTypes);
-        try {
-            for (ShardSearcher subSearcher : subSearchers) {
-                C a = createAggregator(builder, subSearcher, fieldTypes);
-                a.preCollection();
-                subSearcher.search(weight, a);
-                a.postCollection();
-                aggs.add(a.buildAggregation(0L));
-            }
-            if (aggs.isEmpty()) {
-                return null;
-            } else {
-                if (randomBoolean() && aggs.size() > 1) {
-                    // sometimes do an incremental reduce
-                    int toReduceSize = aggs.size();
-                    Collections.shuffle(aggs, random());
-                    int r = randomIntBetween(1, toReduceSize);
-                    List<InternalAggregation> toReduce = aggs.subList(0, r);
-                    A reduced = (A) aggs.get(0).doReduce(toReduce,
-                        new InternalAggregation.ReduceContext(root.context().bigArrays(), null, false));
-                    aggs = new ArrayList<>(aggs.subList(r, toReduceSize));
-                    aggs.add(reduced);
-                }
-                // now do the final reduce
-                @SuppressWarnings("unchecked")
-                A internalAgg = (A) aggs.get(0).doReduce(aggs, new InternalAggregation.ReduceContext(root.context().bigArrays(), null,
-                    true));
-                return internalAgg;
+
+        for (ShardSearcher subSearcher : subSearchers) {
+            C a = createAggregator(builder, subSearcher, fieldTypes);
+            a.preCollection();
+            subSearcher.search(weight, a);
+            a.postCollection();
+            aggs.add(a.buildAggregation(0L));
+        }
+        if (aggs.isEmpty()) {
+            return null;
+        } else {
+            if (randomBoolean() && aggs.size() > 1) {
+                // sometimes do an incremental reduce
+                int toReduceSize = aggs.size();
+                Collections.shuffle(aggs, random());
+                int r = randomIntBetween(1, toReduceSize);
+                List<InternalAggregation> toReduce = aggs.subList(0, r);
+                A reduced = (A) aggs.get(0).doReduce(toReduce,
+                    new InternalAggregation.ReduceContext(root.context().bigArrays(), null, false));
+                aggs = new ArrayList<>(aggs.subList(r, toReduceSize));
+                aggs.add(reduced);
             }
-        } finally {
-            Releasables.close(releasables);
-            releasables.clear();
+            // now do the final reduce
+            @SuppressWarnings("unchecked")
+            A internalAgg = (A) aggs.get(0).doReduce(aggs, new InternalAggregation.ReduceContext(root.context().bigArrays(), null,
+                true));
+            return internalAgg;
         }
+
     }
 
     private static class ShardSearcher extends IndexSearcher {
@@ -300,4 +294,10 @@ public abstract class AggregatorTestCase extends ESTestCase {
     protected static DirectoryReader wrap(DirectoryReader directoryReader) throws IOException {
         return ElasticsearchDirectoryReader.wrap(directoryReader, new ShardId(new Index("_index", "_na_"), 0));
     }
+
+    @After
+    private void cleanupReleasables() {
+        Releasables.close(releasables);
+        releasables.clear();
+    }
 }