diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java index 70e84166c94..51955272b78 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchService.java +++ b/server/src/main/java/org/elasticsearch/search/SearchService.java @@ -626,11 +626,12 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv indexShard.shardId(), request.getClusterAlias(), OriginalIndices.NONE); Engine.Searcher searcher = indexShard.acquireSearcher(source); - final DefaultSearchContext searchContext = new DefaultSearchContext(idGenerator.incrementAndGet(), request, shardTarget, - searcher, clusterService, indexService, indexShard, bigArrays, threadPool::relativeTimeInMillis, timeout, - fetchPhase, clusterService.state().nodes().getMinNodeVersion()); boolean success = false; + DefaultSearchContext searchContext = null; try { + searchContext = new DefaultSearchContext(idGenerator.incrementAndGet(), request, shardTarget, + searcher, clusterService, indexService, indexShard, bigArrays, threadPool::relativeTimeInMillis, timeout, + fetchPhase, clusterService.state().nodes().getMinNodeVersion()); // we clone the query shard context here just for rewriting otherwise we // might end up with incorrect state since we are using now() or script services // during rewrite and normalized / evaluate templates etc. @@ -641,6 +642,11 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv } finally { if (success == false) { IOUtils.closeWhileHandlingException(searchContext); + if (searchContext == null) { + // we handle the case where the DefaultSearchContext constructor throws an exception since we would otherwise + // leak a searcher and this can have severe implications (unable to obtain shard lock exceptions). + IOUtils.closeWhileHandlingException(searcher); + } } } return searchContext; diff --git a/server/src/test/java/org/elasticsearch/search/SearchServiceTests.java b/server/src/test/java/org/elasticsearch/search/SearchServiceTests.java index f8ef11abe9b..81f7fcfef9d 100644 --- a/server/src/test/java/org/elasticsearch/search/SearchServiceTests.java +++ b/server/src/test/java/org/elasticsearch/search/SearchServiceTests.java @@ -30,6 +30,7 @@ import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.search.SearchTask; +import org.elasticsearch.action.search.SearchType; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.action.support.WriteRequest; @@ -680,4 +681,28 @@ public class SearchServiceTests extends ESSingleNodeTestCase { assertSame(searchShardTarget, searchContext.fetchResult().getSearchShardTarget()); } } + + /** + * While we have no NPE in DefaultContext constructor anymore, we still want to guard against it (or other failures) in the future to + * avoid leaking searchers. + */ + public void testCreateSearchContextFailure() throws IOException { + final String index = randomAlphaOfLengthBetween(5, 10).toLowerCase(Locale.ROOT); + final IndexService indexService = createIndex(index); + final SearchService service = getInstanceFromNode(SearchService.class); + final ShardId shardId = new ShardId(indexService.index(), 0); + + NullPointerException e = expectThrows(NullPointerException.class, + () -> service.createContext( + new ShardSearchLocalRequest(shardId, null, 0, null) { + @Override + public SearchType searchType() { + // induce an artificial NPE + throw new NullPointerException("expected"); + } + } + )); + assertEquals("expected", e.getMessage()); + assertEquals("should have 2 store refs (IndexService + InternalEngine)", 2, indexService.getShard(0).store().refCount()); + } }