Skip to content

Commit 9f6042a

Browse files
Make AbstractSearchAsyncAction release resources earlier (#124275)
We can release resources earlier by releasing before responding to the listener (everything that needs retaining is retained via the search response already) as well as make the GCs life a little easier and get obvious correctness by using the listener that nulls out resources in a thread-safe manner intead of a non-thread-safe and mutable list shared across all kinds of places in the code.
1 parent 472536c commit 9f6042a

File tree

2 files changed

+9
-7
lines changed

2 files changed

+9
-7
lines changed

server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import org.elasticsearch.common.util.Maps;
2828
import org.elasticsearch.common.util.concurrent.AtomicArray;
2929
import org.elasticsearch.core.Releasable;
30-
import org.elasticsearch.core.Releasables;
3130
import org.elasticsearch.index.shard.ShardId;
3231
import org.elasticsearch.search.SearchContextMissingException;
3332
import org.elasticsearch.search.SearchPhaseResult;
@@ -101,7 +100,7 @@ abstract class AbstractSearchAsyncAction<Result extends SearchPhaseResult> exten
101100
private final int skippedCount;
102101

103102
// protected for tests
104-
protected final List<Releasable> releasables = new ArrayList<>();
103+
protected final SubscribableListener<Void> doneFuture = new SubscribableListener<>();
105104

106105
AbstractSearchAsyncAction(
107106
String name,
@@ -151,7 +150,7 @@ abstract class AbstractSearchAsyncAction<Result extends SearchPhaseResult> exten
151150
this.executor = executor;
152151
this.request = request;
153152
this.task = task;
154-
this.listener = ActionListener.runAfter(listener, () -> Releasables.close(releasables));
153+
this.listener = ActionListener.runBefore(listener, () -> doneFuture.onResponse(null));
155154
this.nodeIdToConnection = nodeIdToConnection;
156155
this.concreteIndexBoosts = concreteIndexBoosts;
157156
this.clusterStateVersion = clusterState.version();
@@ -190,7 +189,12 @@ protected void notifyListShards(
190189
* Registers a {@link Releasable} that will be closed when the search request finishes or fails.
191190
*/
192191
public void addReleasable(Releasable releasable) {
193-
releasables.add(releasable);
192+
var doneFuture = this.doneFuture;
193+
if (doneFuture.isDone()) {
194+
releasable.close();
195+
} else {
196+
doneFuture.addListener(ActionListener.releasing(releasable));
197+
}
194198
}
195199

196200
/**

server/src/test/java/org/elasticsearch/action/search/MockSearchPhaseContext.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
1919
import org.elasticsearch.common.util.concurrent.AtomicArray;
2020
import org.elasticsearch.core.Nullable;
21-
import org.elasticsearch.core.Releasables;
2221
import org.elasticsearch.search.SearchPhaseResult;
2322
import org.elasticsearch.search.SearchShardTarget;
2423
import org.elasticsearch.search.internal.ShardSearchContextId;
@@ -104,8 +103,7 @@ public void sendSearchResponse(SearchResponseSections internalSearchResponse, At
104103
searchContextId
105104
)
106105
);
107-
Releasables.close(releasables);
108-
releasables.clear();
106+
doneFuture.onResponse(null);
109107
if (existing != null) {
110108
existing.decRef();
111109
}

0 commit comments

Comments
 (0)