Fix several potential circuit breaker leaks in Aggregators by iverase · Pull Request #79676 · elastic/elasticsearch

iverase · 2021-10-22T11:33:55Z

This PR adds a new CircuitBreaker implementation in the test that throws CircuitBreakerExceptions randomly. This new circuit breaker helps uncover several places where we might leak if the circuit breaker throws such exception.

A typical case is an object that creates two big arrays in its constructor. This object itself is created in an aggregator constructor. If the circuit breaker throws an exception during the creation of the second array, we need especial code to handle closing the first array or we leak.

This leaks are probably rare and the effect is small but potentially can decrease the performance of a cluster over time.

elasticmachine · 2021-10-22T11:33:58Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

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

iverase · 2021-10-26T07:33:21Z

@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample

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

nik9000

I wish there was a cleaner looking way to do this. But it does seem right. I left a question about one case that looked different from the others.

nik9000 · 2021-11-03T15:07:34Z

libs/core/src/main/java/org/elasticsearch/core/Releasables.java

    /** Release the provided {@link Releasable}s. */
    public static void close(Releasable... releasables) {
-        close(Arrays.asList(releasables));
+        if (releasables != null) {


It's kind of rude to pass a null list here.

I moved the null check to the composite aggregator that was causing this issue.

nik9000 · 2021-11-03T15:15:06Z

.../src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorFactory.java

+                if (success == false) {
+                    Releasables.close(metricValues);
+                }
+            }


I wonder if it's more clear to do this with one try block.

Changed to one try block in a28511e

nik9000 · 2021-11-03T18:42:14Z

...java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java

+            boolean success = false;
+            try {
                long[] mergeMap = new long[Math.toIntExact(oldOrds.size())];
                bucketOrds = new LongKeyedBucketOrds.FromMany(bigArrays());


I think the problem here is that we may close oldOrds twice, right?

Yes indeed that's the problem. We should not close oldOrds until we successfully allocated bucketOrds andwe need to close it at the end.

# Conflicts: # server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/OrdinalValuesSource.java

iverase · 2021-11-11T09:03:28Z

Hey @nik9000,

Do you think we still want to pursue this change?

nik9000 · 2021-11-22T18:24:47Z

Do you think we still want to pursue this change?

I thought I replied to this last week..... I liked it. let me double check.

nik9000

LGTM

iverase · 2021-11-23T07:20:55Z

@elasticmachine run elasticsearch-ci/part-2

…9676) This commit adds a new CircuitBreaker implementation in the test that throws CircuitBreaker Exceptions randomly. This new circuit breaker helps uncover several places where we might leak if the circuit breaker throws such exception.

elasticsearchmachine · 2021-11-23T08:12:33Z

💚 Backport successful

Status	Branch	Result
✅	8.0
✅	7.16

…80928) This commit adds a new CircuitBreaker implementation in the test that throws CircuitBreaker Exceptions randomly. This new circuit breaker helps uncover several places where we might leak if the circuit breaker throws such exception.

…80929) This commit adds a new CircuitBreaker implementation in the test that throws CircuitBreaker Exceptions randomly. This new circuit breaker helps uncover several places where we might leak if the circuit breaker throws such exception.

nik9000 · 2021-11-23T13:50:50Z

@iverase iverase merged commit 6de5edd into elastic:master 6 hours ago

Thanks for dragging me along with this. I think it's a good change.

iverase · 2021-11-23T14:11:04Z

I do hope that no more leaks can escape our testing :)

* upstream/master: Cleanup SLM History Item .equals (elastic#80938) Rework breaking changes for new structure (elastic#80907) [DOCS] Fix elasticsearch-reset-password typo (elastic#80919) [ML] No need to use parent task client when internal infer delegates (elastic#80905) Fix shadowed vars pt6 (elastic#80899) add ignore info (elastic#80924) Fix several potential circuit breaker leaks in Aggregators (elastic#79676) # Conflicts: # server/src/internalClusterTest/java/org/elasticsearch/index/TimeSeriesModeIT.java

* upstream/master: (29 commits) Fix typo (elastic#80925) Increase docker compose timeouts for CI builds TSDB: fix error without feature flag (elastic#80945) [DOCS] Relocate `index.mapping.dimension_fields.limit` setting docs (elastic#80964) Explicit write methods for always-missing values (elastic#80958) TSDB: move TimeSeriesModeIT to yaml tests (elastic#80933) [ML] Removing temporary debug (elastic#80956) Remove unused ConnectTransportException#node (elastic#80944) Reinterpret dots in field names as object structure (elastic#79922) Remove obsolete typed legacy index templates (elastic#80937) Remove unnecessary shuffle in unassigned shards allocation. (elastic#65172) TSDB: Tests for nanosecond timeprecision timestamp just beyond the limit (elastic#80932) Cleanup SLM History Item .equals (elastic#80938) Rework breaking changes for new structure (elastic#80907) [DOCS] Fix elasticsearch-reset-password typo (elastic#80919) [ML] No need to use parent task client when internal infer delegates (elastic#80905) Fix shadowed vars pt6 (elastic#80899) add ignore info (elastic#80924) Fix several potential circuit breaker leaks in Aggregators (elastic#79676) Extract more standard metadata from binary files (elastic#78754) ...

* upstream/master: (319 commits) Fix typo (elastic#80925) Increase docker compose timeouts for CI builds TSDB: fix error without feature flag (elastic#80945) [DOCS] Relocate `index.mapping.dimension_fields.limit` setting docs (elastic#80964) Explicit write methods for always-missing values (elastic#80958) TSDB: move TimeSeriesModeIT to yaml tests (elastic#80933) [ML] Removing temporary debug (elastic#80956) Remove unused ConnectTransportException#node (elastic#80944) Reinterpret dots in field names as object structure (elastic#79922) Remove obsolete typed legacy index templates (elastic#80937) Remove unnecessary shuffle in unassigned shards allocation. (elastic#65172) TSDB: Tests for nanosecond timeprecision timestamp just beyond the limit (elastic#80932) Cleanup SLM History Item .equals (elastic#80938) Rework breaking changes for new structure (elastic#80907) [DOCS] Fix elasticsearch-reset-password typo (elastic#80919) [ML] No need to use parent task client when internal infer delegates (elastic#80905) Fix shadowed vars pt6 (elastic#80899) add ignore info (elastic#80924) Fix several potential circuit breaker leaks in Aggregators (elastic#79676) Extract more standard metadata from binary files (elastic#78754) ...

Fix several potential Circuit breaker leaks in Aggregators

3aca896

iverase added >bug :Analytics/Aggregations Aggregations v8.0.0 v7.16.1 labels Oct 22, 2021

iverase requested a review from nik9000 October 22, 2021 11:33

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 22, 2021

Merge branch 'master' into AggregatorTestCase

54ce7a4

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

Merge branch 'master' into AggregatorTestCase

cd5fb0f

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

iverase added the v8.1.0 label Nov 2, 2021

nik9000 reviewed Nov 3, 2021

View reviewed changes

iverase added 6 commits November 4, 2021 09:12

Simplify TopMetricsAggregatorFactory

a28511e

add comment

0ae1207

Merge branch 'master' into AggregatorTestCase

cce935e

Remove null check in Releasables

cf4b7c9

Remove null check in Releasables

4caf7d3

Merge branch 'master' into AggregatorTestCase

b10bc2e

# Conflicts: # server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/OrdinalValuesSource.java

nik9000 approved these changes Nov 22, 2021

View reviewed changes

Merge branch 'master' into AggregatorTestCase

f58e028

iverase added the auto-backport-and-merge label Nov 23, 2021

iverase merged commit 6de5edd into elastic:master Nov 23, 2021

iverase deleted the AggregatorTestCase branch November 23, 2021 08:11

iverase mentioned this pull request Nov 23, 2021

[8.0] Fix several potential circuit breaker leaks in Aggregators (#79676) #80928

Merged

iverase mentioned this pull request Nov 23, 2021

[7.16] Fix several potential circuit breaker leaks in Aggregators (#79676) #80929

Merged

danhermann added v7.16.0 and removed v7.16.1 labels Nov 29, 2021

mark-vieira added v8.0.0-rc1 and removed v8.0.0 labels Jan 12, 2022

iverase mentioned this pull request Dec 8, 2022

BigArray memory leaking #91949

Closed

Conversation

iverase commented Oct 22, 2021

Uh oh!

elasticmachine commented Oct 22, 2021

Uh oh!

iverase commented Oct 26, 2021

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

iverase Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

nik9000 Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

iverase Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

nik9000 Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

iverase Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

iverase commented Nov 11, 2021

Uh oh!

nik9000 commented Nov 22, 2021

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

iverase commented Nov 23, 2021

Uh oh!

elasticsearchmachine commented Nov 23, 2021

💚 Backport successful

Uh oh!

nik9000 commented Nov 23, 2021

Uh oh!

iverase commented Nov 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants