Close query cache on index service creation failure

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

Author: DaveCTurner

server/src/main/java/org/elasticsearch/index/IndexModule.java

@@ -39,6 +39,7 @@
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.BigArrays;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
+import org.elasticsearch.core.internal.io.IOUtils;
 import org.elasticsearch.env.NodeEnvironment;
 import org.elasticsearch.index.analysis.AnalysisRegistry;
 import org.elasticsearch.index.cache.query.DisabledQueryCache;
@@ -399,22 +400,31 @@ public IndexService newIndexService(
             indexReaderWrapper.get() == null ? (shard) -> null : indexReaderWrapper.get();
         eventListener.beforeIndexCreated(indexSettings.getIndex(), indexSettings.getSettings());
         final IndexStorePlugin.DirectoryFactory directoryFactory = getDirectoryFactory(indexSettings, directoryFactories);
-        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;
+        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, indexCreationContext, environment, xContentRegistry,
+            final IndexService indexService = new IndexService(indexSettings, indexCreationContext, environment, xContentRegistry,
                 new SimilarityService(indexSettings, scriptService, similarities),
                 shardStoreDeleter, analysisRegistry, engineFactory, circuitBreakerService, bigArrays, threadPool, scriptService,
                 clusterService, client, queryCache, directoryFactory, eventListener, readerWrapperFactory, mapperRegistry,
                 indicesFieldDataCache, searchOperationListeners, indexOperationListeners, namedWriteableRegistry);
+            success = true;
+            return indexService;
+        } finally {
+            if (success == false) {
+                IOUtils.closeWhileHandlingException(queryCache);
+            }
+        }
     }
 
     private static IndexStorePlugin.DirectoryFactory getDirectoryFactory(

server/src/test/java/org/elasticsearch/index/IndexModuleTests.java

@@ -40,6 +40,7 @@
 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;
@@ -84,13 +85,16 @@
 
 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 static java.util.Collections.emptyMap;
 import static org.elasticsearch.index.IndexService.IndexCreationContext.CREATE_INDEX;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.instanceOf;
 
@@ -354,11 +358,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 {
@@ -379,12 +391,29 @@ 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 testMmapNotAllowed() {
         String storeType = randomFrom(IndexModule.Type.HYBRIDFS.getSettingsKey(), IndexModule.Type.MMAPFS.getSettingsKey());
         final Settings settings = Settings.builder()
@@ -403,12 +432,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