Merge pull request #12519 from vespa-engine/bjorncs/remove-pre-binding-of-server-socket

Bjorncs/remove pre binding of server socket

7 files changed, 7 insertions, 370 deletions

diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java
index 3b4475dd11a..959735c4314 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java
@@ -40,9 +40,9 @@ public class ConnectorFactory {
         return connectorConfig;
     }
 
-    public ServerConnector createConnector(final Metric metric, final Server server, final ServerSocketChannel ch) {
+    public ServerConnector createConnector(final Metric metric, final Server server) {
         ServerConnector connector = new JDiscServerConnector(
-                connectorConfig, metric, server, ch, createConnectionFactories(metric).toArray(ConnectionFactory[]::new));
+                connectorConfig, metric, server, createConnectionFactories(metric).toArray(ConnectionFactory[]::new));
         connector.setPort(connectorConfig.listenPort());
         connector.setName(connectorConfig.name());
         connector.setAcceptQueueSize(connectorConfig.acceptQueueSize());
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java
index 5fa43a15912..51bcb892591 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java
@@ -10,39 +10,30 @@ import org.eclipse.jetty.server.ServerConnector;
 
 import javax.servlet.ServletRequest;
 import javax.servlet.http.HttpServletRequest;
-import java.io.IOException;
-import java.lang.reflect.Field;
 import java.net.Socket;
 import java.net.SocketException;
-import java.nio.channels.ServerSocketChannel;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 
 /**
  * @author bjorncs
  */
 class JDiscServerConnector extends ServerConnector {
     public static final String REQUEST_ATTRIBUTE = JDiscServerConnector.class.getName();
-    private final static Logger log = Logger.getLogger(JDiscServerConnector.class.getName());
     private final Metric.Context metricCtx;
     private final Map<RequestDimensions, Metric.Context> requestMetricContextCache = new ConcurrentHashMap<>();
     private final ServerConnectionStatistics statistics;
     private final ConnectorConfig config;
     private final boolean tcpKeepAlive;
     private final boolean tcpNoDelay;
-    private final ServerSocketChannel channelOpenedByActivator;
     private final Metric metric;
     private final String connectorName;
     private final int listenPort;
 
-    JDiscServerConnector(ConnectorConfig config, Metric metric, Server server,
-                         ServerSocketChannel channelOpenedByActivator, ConnectionFactory... factories) {
+    JDiscServerConnector(ConnectorConfig config, Metric metric, Server server, ConnectionFactory... factories) {
         super(server, factories);
-        this.channelOpenedByActivator = channelOpenedByActivator;
         this.config = config;
         this.tcpKeepAlive = config.tcpKeepAliveEnabled();
         this.tcpNoDelay = config.tcpNoDelay();
@@ -69,54 +60,6 @@ class JDiscServerConnector extends ServerConnector {
         }
     }
 
-    @Override
-    public void open() throws IOException {
-        if (channelOpenedByActivator == null) {
-            log.log(Level.INFO, "No channel set by activator, opening channel ourselves.");
-            try {
-                super.open();
-            } catch (RuntimeException e) {
-                log.log(Level.SEVERE, "failed org.eclipse.jetty.server.Server open() with port " + getPort());
-                throw e;
-            }
-            return;
-        }
-        log.log(Level.INFO, "Using channel set by activator: " + channelOpenedByActivator);
-
-        channelOpenedByActivator.socket().setReuseAddress(getReuseAddress());
-        int localPort = channelOpenedByActivator.socket().getLocalPort();
-        try {
-            uglySetLocalPort(localPort);
-        } catch (NoSuchFieldException | IllegalAccessException e) {
-            throw new RuntimeException("Could not set local port.", e);
-        }
-        if (localPort <= 0) {
-            throw new IOException("Server channel not bound");
-        }
-        addBean(channelOpenedByActivator);
-        channelOpenedByActivator.configureBlocking(true);
-        addBean(channelOpenedByActivator);
-
-        try {
-            uglySetChannel(channelOpenedByActivator);
-        } catch (NoSuchFieldException | IllegalAccessException e) {
-            throw new RuntimeException("Could not set server channel.", e);
-        }
-    }
-
-    private void uglySetLocalPort(int localPort) throws NoSuchFieldException, IllegalAccessException {
-        Field localPortField = ServerConnector.class.getDeclaredField("_localPort");
-        localPortField.setAccessible(true);
-        localPortField.set(this, localPort);
-    }
-
-    private void uglySetChannel(ServerSocketChannel channelOpenedByActivator) throws NoSuchFieldException,
-            IllegalAccessException {
-        Field acceptChannelField = ServerConnector.class.getDeclaredField("_acceptChannel");
-        acceptChannelField.setAccessible(true);
-        acceptChannelField.set(this, channelOpenedByActivator);
-    }
-
     public ServerConnectionStatistics getStatistics() {
         return statistics;
     }
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
index a6d2ead951a..71284e09669 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
@@ -8,7 +8,6 @@ import com.yahoo.component.ComponentId;
 import com.yahoo.component.provider.ComponentRegistry;
 import com.yahoo.container.logging.AccessLog;
 import com.yahoo.jdisc.Metric;
-import com.yahoo.jdisc.application.OsgiFramework;
 import com.yahoo.jdisc.http.ConnectorConfig;
 import com.yahoo.jdisc.http.ServerConfig;
 import com.yahoo.jdisc.http.ServletPathsConfig;
@@ -140,7 +139,6 @@ public class JettyHttpServer extends AbstractServerProvider {
             final FilterBindings filterBindings,
             final ComponentRegistry<ConnectorFactory> connectorFactories,
             final ComponentRegistry<ServletHolder> servletHolders,
-            final OsgiFramework osgiFramework,
             final FilterInvoker filterInvoker,
             final AccessLog accessLog) {
         super(container);
@@ -156,12 +154,9 @@ public class JettyHttpServer extends AbstractServerProvider {
         setupJmx(server, serverConfig);
         ((QueuedThreadPool)server.getThreadPool()).setMaxThreads(serverConfig.maxWorkerThreads());
 
-        List<ConnectorConfig> connectorConfigs = new ArrayList<>();
         for (ConnectorFactory connectorFactory : connectorFactories.allComponents()) {
             ConnectorConfig connectorConfig = connectorFactory.getConnectorConfig();
-            connectorConfigs.add(connectorConfig);
-            ServerSocketChannel preBoundChannel = getChannelFromServiceLayer(connectorConfig.listenPort(), osgiFramework.bundleContext());
-            server.addConnector(connectorFactory.createConnector(metric, server, preBoundChannel));
+            server.addConnector(connectorFactory.createConnector(metric, server));
             listenedPorts.add(connectorConfig.listenPort());
         }
 
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java
index 4e38d6dc7fb..07bffffdbbd 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java
@@ -9,12 +9,9 @@ import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.handler.AbstractHandler;
 import org.junit.Test;
 
-import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
-import java.net.InetSocketAddress;
-import java.nio.channels.ServerSocketChannel;
 import java.util.Map;
 
 import static org.hamcrest.CoreMatchers.equalTo;
@@ -25,41 +22,13 @@ import static org.hamcrest.CoreMatchers.equalTo;
 public class ConnectorFactoryTest {
 
     @Test
-    public void requireThatNoPreBoundChannelWorks() throws Exception {
+    public void requireThatServerCanBindChannel() throws Exception {
         Server server = new Server();
         try {
             ConnectorConfig config = new ConnectorConfig(new ConnectorConfig.Builder());
             ConnectorFactory factory = createConnectorFactory(config);
             JDiscServerConnector connector =
-                    (JDiscServerConnector)factory.createConnector(new DummyMetric(), server, null);
-            server.addConnector(connector);
-            server.setHandler(new HelloWorldHandler());
-            server.start();
-
-            SimpleHttpClient client = new SimpleHttpClient(null, connector.getLocalPort(), false);
-            SimpleHttpClient.RequestExecutor ex = client.newGet("/blaasdfnb");
-            SimpleHttpClient.ResponseValidator val = ex.execute();
-            val.expectContent(equalTo("Hello world"));
-        } finally {
-            try {
-                server.stop();
-            } catch (Exception e) {
-                //ignore
-            }
-        }
-    }
-
-    @Test
-    public void requireThatPreBoundChannelWorks() throws Exception {
-        Server server = new Server();
-        try {
-            ServerSocketChannel serverChannel = ServerSocketChannel.open();
-            serverChannel.socket().bind(new InetSocketAddress(0));
-
-            ConnectorConfig config = new ConnectorConfig(new ConnectorConfig.Builder());
-            ConnectorFactory factory = createConnectorFactory(config);
-            JDiscServerConnector connector =
-                    (JDiscServerConnector) factory.createConnector(new DummyMetric(), server, serverChannel);
+                    (JDiscServerConnector)factory.createConnector(new DummyMetric(), server);
             server.addConnector(connector);
             server.setHandler(new HelloWorldHandler());
             server.start();
@@ -83,7 +52,7 @@ public class ConnectorFactoryTest {
 
     private static class HelloWorldHandler extends AbstractHandler {
         @Override
-        public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
+        public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException {
             response.getWriter().write("Hello world");
             response.getWriter().flush();
             response.getWriter().close();
diff --git a/standalone-container/pom.xml b/standalone-container/pom.xml
index 744d6854ab2..b9062b9284c 100644
--- a/standalone-container/pom.xml
+++ b/standalone-container/pom.xml
@@ -90,8 +90,6 @@
             container-disc-jar-with-dependencies.jar,
             vespajlib.jar
           </discPreInstallBundle>
-          <bundleActivator>com.yahoo.container.standalone.StandaloneContainerActivator</bundleActivator>
-          <jdiscPrivilegedActivator>true</jdiscPrivilegedActivator>
         </configuration>
       </plugin>
       <plugin>
diff --git a/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneContainerActivator.java b/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneContainerActivator.java
deleted file mode 100644
index cfd5f753c4f..00000000000
--- a/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneContainerActivator.java
+++ /dev/null
@@ -1,162 +0,0 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.container.standalone;
-
-import com.google.inject.Binder;
-import com.google.inject.Guice;
-import com.google.inject.Module;
-import com.google.inject.util.Modules;
-import com.yahoo.jdisc.application.ContainerActivator;
-import com.yahoo.jdisc.application.ContainerBuilder;
-import com.yahoo.jdisc.application.DeactivatedContainer;
-import com.yahoo.jdisc.application.OsgiFramework;
-import com.yahoo.jdisc.http.ConnectorConfig;
-import com.yahoo.vespa.model.VespaModel;
-import com.yahoo.vespa.model.container.Container;
-import com.yahoo.vespa.model.container.http.ConnectorFactory;
-import com.yahoo.vespa.model.container.http.Http;
-import com.yahoo.vespa.model.container.http.JettyHttpServer;
-import org.osgi.framework.Bundle;
-import org.osgi.framework.BundleActivator;
-import org.osgi.framework.BundleContext;
-
-import java.io.IOException;
-import java.net.InetSocketAddress;
-import java.nio.channels.ServerSocketChannel;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Hashtable;
-import java.util.List;
-import java.util.stream.Collectors;
-
-/**
- * @author Einar M R Rosenvinge
- */
-public class StandaloneContainerActivator implements BundleActivator {
-
-    @Override
-    public void start(BundleContext bundleContext) throws Exception {
-        for (ConnectorConfig config: getConnectorConfigs(getContainer())) {
-            ServerSocketChannel socketChannel = bindChannel(config);
-            registerChannels(bundleContext, config.listenPort(), socketChannel);
-        }
-    }
-
-    static void registerChannels(BundleContext bundleContext, int listenPort, ServerSocketChannel boundChannel) {
-        Hashtable<String, Integer> properties = new Hashtable<>();
-        properties.put("port", listenPort);
-        bundleContext.registerService(ServerSocketChannel.class, boundChannel, properties);
-    }
-
-    static ServerSocketChannel bindChannel(ConnectorConfig channelInfo) throws IOException {
-        ServerSocketChannel serverChannel = ServerSocketChannel.open();
-        InetSocketAddress bindAddress = new InetSocketAddress(channelInfo.listenPort());
-        serverChannel.socket().bind(bindAddress, channelInfo.acceptQueueSize());
-        return serverChannel;
-    }
-
-    @Override
-    public void stop(BundleContext bundleContext) throws Exception { }
-
-    static Container getContainer(Module... modules) {
-        Module activatorModule = new ActivatorModule();
-        List<Module> allModules = new ArrayList<>();
-        allModules.addAll(Arrays.asList(modules));
-        allModules.add(activatorModule);
-
-        StandaloneContainerApplication app = new StandaloneContainerApplication(Guice.createInjector(Modules.combine(allModules)));
-        return app.container();
-    }
-
-    static List<ConnectorConfig> getConnectorConfigs(Container container) {
-        return getConnectorConfigs(container.getHttpServer());
-    }
-
-    private static List<ConnectorConfig> getConnectorConfigs(JettyHttpServer jettyHttpServer) {
-        if (jettyHttpServer == null)
-            return Collections.emptyList();
-
-        return jettyHttpServer.getConnectorFactories().stream().
-                map(StandaloneContainerActivator::getConnectorConfig).
-                collect(Collectors.toList());
-        }
-
-    private static ConnectorConfig getConnectorConfig(ConnectorFactory connectorFactory) {
-        return VespaModel.getConfig(ConnectorConfig.class, connectorFactory);
-    }
-
-
-    private static class ActivatorModule implements Module {
-        @Override
-        public void configure(Binder binder) {
-            binder.bind(OsgiFramework.class).toInstance(new DummyOsgiFramework());
-            binder.bind(ContainerActivator.class).toInstance(new DummyActivatorForStandaloneContainerApp());
-        }
-    }
-
-    private static class DummyActivatorForStandaloneContainerApp implements ContainerActivator {
-        @Override
-        public ContainerBuilder newContainerBuilder() {
-            return new ContainerBuilder(new ArrayList<Module>());
-        }
-
-        @Override
-        public DeactivatedContainer activateContainer(ContainerBuilder builder) {
-            return new DeactivatedContainer() {
-                @Override
-                public Object appContext() {
-                    return new Object();
-                }
-
-                @Override
-                public void notifyTermination(Runnable task) {
-                }
-            };
-        }
-    }
-
-    public static class DummyOsgiFramework implements OsgiFramework {
-        @Override
-        public List<Bundle> installBundle(String bundleLocation) {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public void startBundles(List<Bundle> bundles, boolean privileged) {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public void refreshPackages() {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public BundleContext bundleContext() {
-            return null;
-        }
-
-        @Override
-        public List<Bundle> bundles() {
-            return Collections.emptyList();
-        }
-
-        @Override
-        public List<Bundle> getBundles(Bundle requestingBundle) {
-            return Collections.emptyList();
-        }
-
-        @Override
-        public void allowDuplicateBundles(Collection<Bundle> bundles) { }
-
-        @Override
-        public void start() {
-        }
-
-        @Override
-        public void stop() {
-        }
-    }
-
-}
diff --git a/standalone-container/src/test/java/com/yahoo/container/standalone/StandaloneContainerActivatorTest.java b/standalone-container/src/test/java/com/yahoo/container/standalone/StandaloneContainerActivatorTest.java
deleted file mode 100644
index f1c02b0149f..00000000000
--- a/standalone-container/src/test/java/com/yahoo/container/standalone/StandaloneContainerActivatorTest.java
+++ /dev/null
@@ -1,106 +0,0 @@
-// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.container.standalone;
-
-import com.google.inject.Module;
-import com.yahoo.io.IOUtils;
-import com.yahoo.jdisc.http.ConnectorConfig;
-import com.yahoo.vespa.defaults.Defaults;
-import com.yahoo.vespa.model.container.Container;
-import org.junit.Test;
-import org.xml.sax.SAXException;
-
-import javax.xml.parsers.ParserConfigurationException;
-import java.io.FileWriter;
-import java.io.IOException;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.util.List;
-
-import static java.util.Arrays.asList;
-import static java.util.Collections.singletonList;
-import static java.util.stream.Collectors.toList;
-import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.collection.IsEmptyCollection.empty;
-import static org.junit.Assert.assertThat;
-
-/**
- * @author Einar M R Rosenvinge
- */
-public class StandaloneContainerActivatorTest {
-
-    private static String getJdiscXml(String contents) throws ParserConfigurationException, IOException, SAXException {
-        return "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" +
-                "<services>\n" +
-                "  <container version=\"1.0\" jetty=\"true\">\n" +
-                contents +
-                "  </container>\n" +
-                "</services>";
-    }
-
-    private static void writeApplicationPackage(String servicesXml, Path tmpDir) throws IOException {
-        FileWriter fw = new FileWriter(tmpDir.resolve("services.xml").toFile(), false);
-        fw.write(servicesXml);
-        fw.close();
-    }
-
-    @Test
-    public void requireThatPortsCanBeFoundBasic() throws IOException, ParserConfigurationException, SAXException {
-        final Path applicationDir = Files.createTempDirectory("application");
-        try {
-            writeApplicationPackage(getJdiscXml(""), applicationDir);
-            StandaloneContainerActivator activator = new StandaloneContainerActivator();
-            Container container = StandaloneContainerActivator.getContainer(newAppDirBinding(applicationDir));
-            List<Integer> ports = getPorts(activator, container);
-            assertThat(ports, is(singletonList(Defaults.getDefaults().vespaWebServicePort())));
-        } finally {
-            IOUtils.recursiveDeleteDir(applicationDir.toFile());
-        }
-    }
-
-    private static List<Integer> getPorts(StandaloneContainerActivator activator, Container container) {
-        return StandaloneContainerActivator.getConnectorConfigs(container).stream().
-                map(ConnectorConfig::listenPort).
-                collect(toList());
-    }
-
-    @Test
-    public void requireThatPortsCanBeFoundNoHttp() throws IOException, ParserConfigurationException, SAXException {
-        final Path applicationDir = Files.createTempDirectory("application");
-        try {
-            writeApplicationPackage(getJdiscXml("<http/>"), applicationDir);
-            StandaloneContainerActivator activator = new StandaloneContainerActivator();
-            Container container = StandaloneContainerActivator.getContainer(newAppDirBinding(applicationDir));
-            List<Integer> ports = getPorts(activator, container);
-            assertThat(ports, empty());
-        } finally {
-            IOUtils.recursiveDeleteDir(applicationDir.toFile());
-        }
-    }
-
-    @Test
-    public void requireThatPortsCanBeFoundHttpThreeServers() throws IOException, ParserConfigurationException, SAXException {
-        final Path applicationDir = Files.createTempDirectory("application");
-        try {
-            final String contents =
-                    "<http>\n" +
-                    "  <server id=\"a\" port=\"123\"/>\n" +
-                    "  <server id=\"b\" port=\"456\"/>\n" +
-                    "  <server id=\"c\" port=\"789\"/>\n" +
-                    "</http>\n";
-            writeApplicationPackage(getJdiscXml(contents), applicationDir);
-            StandaloneContainerActivator activator = new StandaloneContainerActivator();
-            Container container = StandaloneContainerActivator.getContainer(newAppDirBinding(applicationDir));
-            List<Integer> ports = getPorts(activator, container);
-            assertThat(ports, is(asList(123, 456, 789)));
-        } finally {
-            IOUtils.recursiveDeleteDir(applicationDir.toFile());
-        }
-    }
-
-    private static Module newAppDirBinding(final Path applicationDir) {
-        return binder -> binder.bind(Path.class)
-                .annotatedWith(StandaloneContainerApplication.APPLICATION_PATH_NAME)
-                .toInstance(applicationDir);
-    }
-
-}