Detach BigArrays from Guice (#18973)

BigArrays can be fully constructed without Guice, this change cleans up
it's creation and the mocking in MockNode.

core/src/main/java/org/elasticsearch/client/transport/TransportClient.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.client.transport;
 
+import org.apache.lucene.util.IOUtils;
 import org.elasticsearch.action.Action;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.ActionModule;
@@ -41,7 +42,6 @@ import org.elasticsearch.common.settings.SettingsModule;
 import org.elasticsearch.common.transport.TransportAddress;
 import org.elasticsearch.common.util.BigArrays;
 import org.elasticsearch.indices.breaker.CircuitBreakerService;
-import org.elasticsearch.monitor.MonitorService;
 import org.elasticsearch.node.Node;
 import org.elasticsearch.node.internal.InternalSettingsPreparer;
 import org.elasticsearch.plugins.Plugin;
@@ -52,6 +52,7 @@ import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.transport.TransportService;
 import org.elasticsearch.transport.netty.NettyTransport;
 
+import java.io.Closeable;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
@@ -118,10 +119,11 @@ public class TransportClient extends AbstractClient {
         public TransportClient build() {
             final PluginsService pluginsService = newPluginService(providedSettings);
             final Settings settings = pluginsService.updatedSettings();
+            final List<Closeable> resourcesToClose = new ArrayList<>();
             final ThreadPool threadPool = new ThreadPool(settings);
+            resourcesToClose.add(() -> ThreadPool.terminate(threadPool, 10, TimeUnit.SECONDS));
             final NetworkService networkService = new NetworkService(settings);
             NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry();
-            boolean success = false;
             try {
                 ModulesBuilder modules = new ModulesBuilder();
                 // plugin modules must be added here, before others or we can get crazy injection errors...
@@ -149,8 +151,12 @@ public class TransportClient extends AbstractClient {
                 SettingsModule settingsModule = new SettingsModule(settings, additionalSettings, additionalSettingsFilter);
                 CircuitBreakerService circuitBreakerService = Node.createCircuitBreakerService(settingsModule.getSettings(),
                     settingsModule.getClusterSettings());
+                resourcesToClose.add(circuitBreakerService);
+                BigArrays bigArrays = new BigArrays(settings, circuitBreakerService);
+                resourcesToClose.add(bigArrays);
                 modules.add(settingsModule);
                 modules.add((b -> {
+                    b.bind(BigArrays.class).toInstance(bigArrays);
                     b.bind(PluginsService.class).toInstance(pluginsService);
                     b.bind(CircuitBreakerService.class).toInstance(circuitBreakerService);
                 }));
@@ -159,14 +165,11 @@ public class TransportClient extends AbstractClient {
                 final TransportService transportService = injector.getInstance(TransportService.class);
                 transportService.start();
                 transportService.acceptIncomingRequests();
-
                 TransportClient transportClient = new TransportClient(injector);
-                success = true;
+                resourcesToClose.clear();
                 return transportClient;
             } finally {
-                if (!success) {
-                    ThreadPool.terminate(threadPool, 10, TimeUnit.SECONDS);
-                }
+                IOUtils.closeWhileHandlingException(resourcesToClose);
             }
         }
     }
@@ -261,24 +264,16 @@ public class TransportClient extends AbstractClient {
      */
     @Override
     public void close() {
-        injector.getInstance(TransportClientNodesService.class).close();
-        injector.getInstance(TransportService.class).close();
-        try {
-            injector.getInstance(MonitorService.class).close();
-        } catch (Exception e) {
-            // ignore, might not be bounded
-        }
+        List<Closeable> closeables = new ArrayList<>();
+        closeables.add(injector.getInstance(TransportClientNodesService.class));
+        closeables.add(injector.getInstance(TransportService.class));
 
         for (Class<? extends LifecycleComponent> plugin : injector.getInstance(PluginsService.class).nodeServices()) {
-            injector.getInstance(plugin).close();
+            closeables.add(injector.getInstance(plugin));
         }
-        try {
-            ThreadPool.terminate(injector.getInstance(ThreadPool.class), 10, TimeUnit.SECONDS);
-        } catch (Exception e) {
-            // ignore
-        }
-
-        injector.getInstance(BigArrays.class).close();
+        closeables.add(() -> ThreadPool.terminate(injector.getInstance(ThreadPool.class), 10, TimeUnit.SECONDS));
+        closeables.add(injector.getInstance(BigArrays.class));
+        IOUtils.closeWhileHandlingException(closeables);
     }
 
     @Override

core/src/main/java/org/elasticsearch/client/transport/TransportClientNodesService.java

@@ -48,6 +48,7 @@ import org.elasticsearch.transport.TransportException;
 import org.elasticsearch.transport.TransportRequestOptions;
 import org.elasticsearch.transport.TransportService;
 
+import java.io.Closeable;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
@@ -66,7 +67,7 @@ import static org.elasticsearch.common.unit.TimeValue.timeValueSeconds;
 /**
  *
  */
-public class TransportClientNodesService extends AbstractComponent {
+public class TransportClientNodesService extends AbstractComponent implements Closeable {
 
     private final TimeValue nodesSamplerInterval;
 

core/src/main/java/org/elasticsearch/common/util/BigArrays.java

@@ -373,12 +373,11 @@ public class BigArrays implements Releasable {
     final boolean checkBreaker;
     private final BigArrays circuitBreakingInstance;
 
-    @Inject
     public BigArrays(Settings settings, @Nullable final CircuitBreakerService breakerService) {
         // Checking the breaker is disabled if not specified
         this(new PageCacheRecycler(settings), breakerService, false);
     }
-
+    // public for tests
     public BigArrays(PageCacheRecycler recycler, @Nullable final CircuitBreakerService breakerService, boolean checkBreaker) {
         this.checkBreaker = checkBreaker;
         this.recycler = recycler;

core/src/main/java/org/elasticsearch/env/NodeEnvironment.java

@@ -32,12 +32,9 @@ import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.common.SuppressForbidden;
-import org.elasticsearch.common.UUIDs;
 import org.elasticsearch.common.component.AbstractComponent;
-import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.io.FileSystemUtils;
 import org.elasticsearch.common.logging.DeprecationLogger;
-import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Setting.Property;
 import org.elasticsearch.common.settings.Settings;
@@ -66,7 +63,6 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -167,7 +163,6 @@ public final class NodeEnvironment extends AbstractComponent implements Closeabl
     public static final String NODE_LOCK_FILENAME = "node.lock";
     public static final String UPGRADE_LOCK_FILENAME = "upgrade.lock";
 
-    @Inject
     public NodeEnvironment(Settings settings, Environment environment) throws IOException {
         super(settings);
 

core/src/main/java/org/elasticsearch/node/Node.java

@@ -266,6 +266,8 @@ public class Node implements Closeable {
             CircuitBreakerService circuitBreakerService = createCircuitBreakerService(settingsModule.getSettings(),
                 settingsModule.getClusterSettings());
             resourcesToClose.add(circuitBreakerService);
+            BigArrays bigArrays = createBigArrays(settings, circuitBreakerService);
+            resourcesToClose.add(bigArrays);
             modules.add(settingsModule);
             modules.add(b -> {
                     b.bind(PluginsService.class).toInstance(pluginsService);
@@ -276,6 +278,7 @@ public class Node implements Closeable {
                     b.bind(TribeService.class).toInstance(tribeService);
                     b.bind(ResourceWatcherService.class).toInstance(resourceWatcherService);
                     b.bind(CircuitBreakerService.class).toInstance(circuitBreakerService);
+                    b.bind(BigArrays.class).toInstance(bigArrays);
                 }
             );
             injector = modules.createInjector();
@@ -626,4 +629,12 @@ public class Node implements Closeable {
             throw new IllegalArgumentException("Unknown circuit breaker type [" + type + "]");
         }
     }
+
+    /**
+     * Creates a new {@link BigArrays} instance used for this node.
+     * This method can be overwritten by subclasses to change their {@link BigArrays} implementation for instance for testing
+     */
+    BigArrays createBigArrays(Settings settings, CircuitBreakerService circuitBreakerService) {
+        return new BigArrays(settings, circuitBreakerService);
+    }
 }

core/src/main/java/org/elasticsearch/node/NodeModule.java

@@ -37,9 +37,6 @@ public class NodeModule extends AbstractModule {
     private final MonitorService monitorService;
     private final ProcessorsRegistry.Builder processorsRegistryBuilder;
 
-    // pkg private so tests can mock
-    Class<? extends BigArrays> bigArraysImpl = BigArrays.class;
-
     public NodeModule(Node node, MonitorService monitorService) {
         this.node = node;
         this.monitorService = monitorService;
@@ -48,12 +45,6 @@ public class NodeModule extends AbstractModule {
 
     @Override
     protected void configure() {
-        if (bigArraysImpl == BigArrays.class) {
-            bind(BigArrays.class).asEagerSingleton();
-        } else {
-            bind(BigArrays.class).to(bigArraysImpl).asEagerSingleton();
-        }
-
         bind(Node.class).toInstance(node);
         bind(MonitorService.class).toInstance(monitorService);
         bind(NodeService.class).asEagerSingleton();

test/framework/src/main/java/org/elasticsearch/common/util/MockBigArrays.java

@@ -74,7 +74,6 @@ public class MockBigArrays extends BigArrays {
     private final PageCacheRecycler recycler;
     private final CircuitBreakerService breakerService;
 
-    @Inject
     public MockBigArrays(Settings settings, CircuitBreakerService breakerService) {
         this(new MockPageCacheRecycler(settings), breakerService, false);
     }

test/framework/src/main/java/org/elasticsearch/node/MockNode.java

@@ -19,8 +19,10 @@
 
 package org.elasticsearch.node;
 
-import org.elasticsearch.Version;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.util.BigArrays;
+import org.elasticsearch.common.util.MockBigArrays;
+import org.elasticsearch.indices.breaker.CircuitBreakerService;
 import org.elasticsearch.node.internal.InternalSettingsPreparer;
 import org.elasticsearch.plugins.Plugin;
 
@@ -35,15 +37,25 @@ import java.util.Collection;
  */
 public class MockNode extends Node {
 
+    private final boolean mockBigArrays;
     private Collection<Class<? extends Plugin>> plugins;
 
     public MockNode(Settings settings, Collection<Class<? extends Plugin>> classpathPlugins) {
         super(InternalSettingsPreparer.prepareEnvironment(settings, null), classpathPlugins);
         this.plugins = classpathPlugins;
+        this.mockBigArrays = classpathPlugins.contains(NodeMocksPlugin.class); // if this plugin is present we mock bigarrays :)
     }
 
     public Collection<Class<? extends Plugin>> getPlugins() {
         return plugins;
     }
 
+    @Override
+    protected BigArrays createBigArrays(Settings settings, CircuitBreakerService circuitBreakerService) {
+        if (mockBigArrays) {
+            return new MockBigArrays(settings, circuitBreakerService);
+        } else {
+            return super.createBigArrays(settings, circuitBreakerService);
+        }
+    }
 }

test/framework/src/main/java/org/elasticsearch/node/NodeMocksPlugin.java

@@ -18,12 +18,7 @@
  */
 package org.elasticsearch.node;
 
-import org.elasticsearch.common.util.MockBigArrays;
 import org.elasticsearch.plugins.Plugin;
 
-public class NodeMocksPlugin extends Plugin {
-
-    public void onModule(NodeModule module) {
-        module.bigArraysImpl = MockBigArrays.class;
-    }
+public class NodeMocksPlugin extends Plugin { // just a marker plugin for MockNode to mock out BigArrays
 }

test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java

@@ -902,9 +902,12 @@ public final class InternalTestCluster extends TestCluster {
 
         @Override
         public void close() throws IOException {
-            resetClient();
-            closed.set(true);
-            closeNode();
+            try {
+                resetClient();
+            } finally {
+                closed.set(true);
+                closeNode();
+            }
         }
     }
 

test/framework/src/main/java/org/elasticsearch/test/NodeConfigurationSource.java

@@ -19,18 +19,10 @@
 package org.elasticsearch.test;
 
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.index.MockEngineFactoryPlugin;
-import org.elasticsearch.node.NodeMocksPlugin;
 import org.elasticsearch.plugins.Plugin;
-import org.elasticsearch.search.MockSearchService;
-import org.elasticsearch.test.store.MockFSIndexStore;
-import org.elasticsearch.test.transport.AssertingLocalTransport;
-import org.elasticsearch.test.transport.MockTransportService;
 
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.List;
 
 public abstract class NodeConfigurationSource {