Skip to content

Commit

Permalink
Close query cache on index service creation failure (elastic#48230)
Browse files Browse the repository at this point in the history 
Today it is possible that we create the `QueryCache` and then fail to create
the owning `IndexService` and this means we do not close the `QueryCache`
again. This commit addresses that leak.

Fixes elastic#48186
  • Loading branch information
@DaveCTurner
DaveCTurner committed Oct 21, 2019
1 parent 20d50c5 commit d927b2a
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 20 deletions.
35 changes: 24 additions & 11 deletions server/src/main/java/org/elasticsearch/index/IndexModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.lucene.search.similarities.Similarity;
import org.apache.lucene.store.MMapDirectory;
import org.apache.lucene.util.Constants;
import org.apache.lucene.util.IOUtils;
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.Version;
import org.elasticsearch.client.Client;
Expand All @@ -35,6 +36,7 @@
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.index.analysis.AnalysisRegistry;
import org.elasticsearch.index.analysis.IndexAnalyzers;
import org.elasticsearch.index.cache.query.DisabledQueryCache;
import org.elasticsearch.index.cache.query.IndexQueryCache;
import org.elasticsearch.index.cache.query.QueryCache;
Expand Down Expand Up @@ -388,22 +390,33 @@ public IndexService newIndexService(
? (shard) -> null : indexSearcherWrapper.get();
eventListener.beforeIndexCreated(indexSettings.getIndex(), indexSettings.getSettings());
final IndexStore store = getIndexStore(indexSettings, indexStoreFactories);
final QueryCache queryCache;
if (indexSettings.getValue(INDEX_QUERY_CACHE_ENABLED_SETTING)) {
BiFunction<IndexSettings, IndicesQueryCache, QueryCache> queryCacheProvider = forceQueryCacheProvider.get();
if (queryCacheProvider == null) {
queryCache = new IndexQueryCache(indexSettings, indicesQueryCache);
QueryCache queryCache = null;
IndexAnalyzers indexAnalyzers = null;
boolean success = false;
try {
if (indexSettings.getValue(INDEX_QUERY_CACHE_ENABLED_SETTING)) {
BiFunction<IndexSettings, IndicesQueryCache, QueryCache> queryCacheProvider = forceQueryCacheProvider.get();
if (queryCacheProvider == null) {
queryCache = new IndexQueryCache(indexSettings, indicesQueryCache);
} else {
queryCache = queryCacheProvider.apply(indexSettings, indicesQueryCache);
}
} else {
queryCache = queryCacheProvider.apply(indexSettings, indicesQueryCache);
queryCache = new DisabledQueryCache(indexSettings);
}
} else {
queryCache = new DisabledQueryCache(indexSettings);
}
return new IndexService(indexSettings, environment, xContentRegistry,
indexAnalyzers = analysisRegistry.build(indexSettings);
final IndexService indexService = new IndexService(indexSettings, environment, xContentRegistry,
new SimilarityService(indexSettings, scriptService, similarities),
shardStoreDeleter, analysisRegistry, engineFactory, circuitBreakerService, bigArrays, threadPool, scriptService,
shardStoreDeleter, indexAnalyzers, engineFactory, circuitBreakerService, bigArrays, threadPool, scriptService,
client, queryCache, store, eventListener, searcherWrapperFactory, mapperRegistry,
indicesFieldDataCache, searchOperationListeners, indexOperationListeners, namedWriteableRegistry);
success = true;
return indexService;
} finally {
if (success == false) {
IOUtils.closeWhileHandlingException(queryCache, indexAnalyzers);
}
}
}

private static IndexStore getIndexStore(
Expand Down
8 changes: 3 additions & 5 deletions server/src/main/java/org/elasticsearch/index/IndexService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.env.ShardLock;
import org.elasticsearch.env.ShardLockObtainFailedException;
import org.elasticsearch.index.analysis.AnalysisRegistry;
import org.elasticsearch.index.analysis.IndexAnalyzers;
import org.elasticsearch.index.cache.IndexCache;
import org.elasticsearch.index.cache.bitset.BitsetFilterCache;
Expand Down Expand Up @@ -142,8 +141,7 @@ public IndexService(
NamedXContentRegistry xContentRegistry,
SimilarityService similarityService,
ShardStoreDeleter shardStoreDeleter,
AnalysisRegistry registry,
EngineFactory engineFactory,
IndexAnalyzers indexAnalyzers, EngineFactory engineFactory,
CircuitBreakerService circuitBreakerService,
BigArrays bigArrays,
ThreadPool threadPool,
Expand All @@ -157,14 +155,14 @@ public IndexService(
IndicesFieldDataCache indicesFieldDataCache,
List<SearchOperationListener> searchOperationListeners,
List<IndexingOperationListener> indexingOperationListeners,
NamedWriteableRegistry namedWriteableRegistry) throws IOException {
NamedWriteableRegistry namedWriteableRegistry) {
super(indexSettings);
this.indexSettings = indexSettings;
this.xContentRegistry = xContentRegistry;
this.similarityService = similarityService;
this.namedWriteableRegistry = namedWriteableRegistry;
this.circuitBreakerService = circuitBreakerService;
this.mapperService = new MapperService(indexSettings, registry.build(indexSettings), xContentRegistry, similarityService,
this.mapperService = new MapperService(indexSettings, indexAnalyzers, xContentRegistry, similarityService,
mapperRegistry,
// we parse all percolator queries as they would be parsed on shard 0
() -> newQueryShardContext(0, null, System::currentTimeMillis, null));
Expand Down
93 changes: 89 additions & 4 deletions server/src/test/java/org/elasticsearch/index/IndexModuleTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
package org.elasticsearch.index;

import org.apache.lucene.analysis.Analyzer;
import org.apache.lucene.index.AssertingDirectoryReader;
import org.apache.lucene.index.DirectoryReader;
import org.apache.lucene.index.FieldInvertState;
Expand All @@ -40,12 +41,15 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.common.util.PageCacheRecycler;
import org.elasticsearch.common.util.concurrent.EsRejectedExecutionException;
import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.env.ShardLock;
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.index.analysis.AnalysisRegistry;
import org.elasticsearch.index.analysis.AnalyzerProvider;
import org.elasticsearch.index.analysis.AnalyzerScope;
import org.elasticsearch.index.cache.query.DisabledQueryCache;
import org.elasticsearch.index.cache.query.IndexQueryCache;
import org.elasticsearch.index.cache.query.QueryCache;
Expand All @@ -65,6 +69,7 @@
import org.elasticsearch.index.store.IndexStore;
import org.elasticsearch.indices.IndicesModule;
import org.elasticsearch.indices.IndicesQueryCache;
import org.elasticsearch.indices.analysis.AnalysisModule;
import org.elasticsearch.indices.breaker.CircuitBreakerService;
import org.elasticsearch.indices.breaker.NoneCircuitBreakerService;
import org.elasticsearch.indices.cluster.IndicesClusterStateService.AllocatedIndices.IndexRemovalReason;
Expand All @@ -83,13 +88,17 @@

import java.io.IOException;
import java.util.Collections;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Function;

import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonMap;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.hasToString;
import static org.hamcrest.Matchers.instanceOf;

Expand Down Expand Up @@ -351,11 +360,19 @@ public void testForceCustomQueryCache() throws IOException {
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).build();
final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("foo", settings);
IndexModule module = new IndexModule(indexSettings, emptyAnalysisRegistry, new InternalEngineFactory(), Collections.emptyMap());
module.forceQueryCacheProvider((a, b) -> new CustomQueryCache());
expectThrows(AlreadySetException.class, () -> module.forceQueryCacheProvider((a, b) -> new CustomQueryCache()));
final Set<CustomQueryCache> liveQueryCaches = new HashSet<>();
module.forceQueryCacheProvider((a, b) -> {
final CustomQueryCache customQueryCache = new CustomQueryCache(liveQueryCaches);
liveQueryCaches.add(customQueryCache);
return customQueryCache;
});
expectThrows(AlreadySetException.class, () -> module.forceQueryCacheProvider((a, b) -> {
throw new AssertionError("never called");
}));
IndexService indexService = newIndexService(module);
assertTrue(indexService.cache().query() instanceof CustomQueryCache);
indexService.close("simon says", false);
assertThat(liveQueryCaches, empty());
}

public void testDefaultQueryCacheImplIsSelected() throws IOException {
Expand All @@ -376,12 +393,73 @@ public void testDisableQueryCacheHasPrecedenceOverForceQueryCache() throws IOExc
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).build();
final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("foo", settings);
IndexModule module = new IndexModule(indexSettings, emptyAnalysisRegistry, new InternalEngineFactory(), Collections.emptyMap());
module.forceQueryCacheProvider((a, b) -> new CustomQueryCache());
module.forceQueryCacheProvider((a, b) -> new CustomQueryCache(null));
IndexService indexService = newIndexService(module);
assertTrue(indexService.cache().query() instanceof DisabledQueryCache);
indexService.close("simon says", false);
}

public void testCustomQueryCacheCleanedUpIfIndexServiceCreationFails() {
Settings settings = Settings.builder()
.put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString())
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).build();
final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("foo", settings);
IndexModule module = new IndexModule(indexSettings, emptyAnalysisRegistry, new InternalEngineFactory(), Collections.emptyMap());
final Set<CustomQueryCache> liveQueryCaches = new HashSet<>();
module.forceQueryCacheProvider((a, b) -> {
final CustomQueryCache customQueryCache = new CustomQueryCache(liveQueryCaches);
liveQueryCaches.add(customQueryCache);
return customQueryCache;
});
threadPool.shutdown(); // causes index service creation to fail
expectThrows(EsRejectedExecutionException.class, () -> newIndexService(module));
assertThat(liveQueryCaches, empty());
}

public void testIndexAnalyzersCleanedUpIfIndexServiceCreationFails() {
Settings settings = Settings.builder()
.put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString())
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).build();
final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("foo", settings);

final HashSet<Analyzer> openAnalyzers = new HashSet<>();
final AnalysisModule.AnalysisProvider<AnalyzerProvider<?>> analysisProvider = (i,e,n,s) -> new AnalyzerProvider<Analyzer>() {
@Override
public String name() {
return "test";
}

@Override
public AnalyzerScope scope() {
return AnalyzerScope.INDEX;
}

@Override
public Analyzer get() {
final Analyzer analyzer = new Analyzer() {
@Override
protected TokenStreamComponents createComponents(String fieldName) {
throw new AssertionError("should not be here");
}

@Override
public void close() {
super.close();
openAnalyzers.remove(this);
}
};
openAnalyzers.add(analyzer);
return analyzer;
}
};
final AnalysisRegistry analysisRegistry = new AnalysisRegistry(environment, emptyMap(), emptyMap(), emptyMap(),
singletonMap("test", analysisProvider), emptyMap(), emptyMap(), emptyMap(), emptyMap(), emptyMap());
IndexModule module = new IndexModule(indexSettings, analysisRegistry, new InternalEngineFactory(), Collections.emptyMap());
threadPool.shutdown(); // causes index service creation to fail
expectThrows(EsRejectedExecutionException.class, () -> newIndexService(module));
assertThat(openAnalyzers, empty());
}

public void testMmapNotAllowed() {
String storeType = randomFrom(IndexModule.Type.HYBRIDFS.getSettingsKey(), IndexModule.Type.MMAPFS.getSettingsKey());
final Settings settings = Settings.builder()
Expand All @@ -400,12 +478,19 @@ public void testMmapNotAllowed() {

class CustomQueryCache implements QueryCache {

private final Set<CustomQueryCache> liveQueryCaches;

CustomQueryCache(Set<CustomQueryCache> liveQueryCaches) {
this.liveQueryCaches = liveQueryCaches;
}

@Override
public void clear(String reason) {
}

@Override
public void close() throws IOException {
public void close() {
assertTrue(liveQueryCaches == null || liveQueryCaches.remove(this));
}

@Override
Expand Down

0 comments on commit d927b2a

Please sign in to comment.