honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix SearchService.createContext exception handling (#46258) 

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
This commit is contained in:
Henning Andersen 2019-09-04 14:07:26 +02:00 committed by Henning Andersen
parent c71d959d61
commit 5066835569
2 changed files with 34 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
server/src/main/java/org/elasticsearch/search/SearchService.java
View File

@ -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;

25
server/src/test/java/org/elasticsearch/search/SearchServiceTests.java
View File

@ -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());
}
}