From f7a9725f1b26e5e2777493ce01578f8ad6cdbdbc Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Tue, 3 Sep 2019 12:11:03 +0200 Subject: [PATCH 1/2] Fix SearchService.createContext exception handling An exception from the DefaultSearchContext constructor could leak a searcher, causing future issues like shard lock obtained exceptions. The underlying cause of the exception in the constructor has been fixed, but as a safety precaution we also fix the exception handling in createContext. Closes #45378 --- .../elasticsearch/search/SearchService.java | 10 +++++--- .../search/SearchServiceTests.java | 25 +++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java index 83e15eb5e73f3..e04904b4c158e 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 @@ private DefaultSearchContext createSearchContext(ShardSearchRequest request, Tim 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,9 @@ private DefaultSearchContext createSearchContext(ShardSearchRequest request, Tim } finally { if (success == false) { IOUtils.closeWhileHandlingException(searchContext); + if (searchContext == null) { + 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 f8ef11abe9bf4..dc3f91f4502a5 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.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 void testCreateSearchContext() throws IOException { 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, 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()); + } } From 2ebfc6cb33434cf0005780348d01089a4376ee77 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Wed, 4 Sep 2019 11:31:24 +0200 Subject: [PATCH 2/2] Add comment to explain why --- .../src/main/java/org/elasticsearch/search/SearchService.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java index e04904b4c158e..9311ae287d9a8 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchService.java +++ b/server/src/main/java/org/elasticsearch/search/SearchService.java @@ -643,6 +643,8 @@ private DefaultSearchContext createSearchContext(ShardSearchRequest request, Tim 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); } }