Refactor Http to be thread-safe and more easily mutable

Guard access to internal state with monitor. Make access control field mutable.
Make null return value in getters explicit with Optional in return type.
Take filter chains in constructor. Simplify modifications to http bindings.

6 files changed, 79 insertions, 77 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java b/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java
index 7e2d6680827..764b42c0ea2 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java
@@ -140,7 +140,7 @@ public abstract class Container extends AbstractService implements
         if (http == null) {
             return defaultHttpServer;
         } else {
-            return http.getHttpServer();
+            return http.getHttpServer().orElse(null);
         }
     }
 
@@ -228,10 +228,10 @@ public abstract class Container extends AbstractService implements
             // XXX unused - remove:
             from.allocatePort("http/1");
             portsMeta.on(offset++).tag("http").tag("external");
-        } else if (getHttp().getHttpServer() == null) {
+        } else if (getHttp().getHttpServer().isEmpty()) {
             // no http server ports
         } else {
-            for (ConnectorFactory connectorFactory : getHttp().getHttpServer().getConnectorFactories()) {
+            for (ConnectorFactory connectorFactory : getHttp().getHttpServer().get().getConnectorFactories()) {
                 int port = getPort(connectorFactory);
                 String name = "http/" + connectorFactory.getName();
                 from.requirePort(port, name);
@@ -280,7 +280,7 @@ public abstract class Container extends AbstractService implements
         final Http http = getHttp();
         if (http != null) {
             // TODO: allow the user to specify health port manually
-            if (http.getHttpServer() == null) {
+            if (http.getHttpServer().isEmpty()) {
                 return -1;
             } else {
                 return getRelativePort(0);
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java
index f3758def2b1..9b8d138a98a 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java
@@ -116,8 +116,8 @@ public class ConfigserverCluster extends AbstractConfigProducer
         }
 
         builder.serverId(HostName.getLocalhost());
-        if (!containerCluster.getHttp().getHttpServer().getConnectorFactories().isEmpty()) {
-            builder.httpport(containerCluster.getHttp().getHttpServer().getConnectorFactories().get(0).getListenPort());
+        if (!containerCluster.getHttp().getHttpServer().get().getConnectorFactories().isEmpty()) {
+            builder.httpport(containerCluster.getHttp().getHttpServer().get().getConnectorFactories().get(0).getListenPort());
         }
         if (options.useVespaVersionInRequest().isPresent()) {
             builder.useVespaVersionInRequest(options.useVespaVersionInRequest().get());
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java
index 400ddf80cf9..4742ebfcf89 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java
@@ -8,104 +8,112 @@ import com.yahoo.vespa.model.container.component.chain.Chain;
 import com.yahoo.vespa.model.container.component.chain.ChainedComponent;
 
 import java.util.Collection;
-import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 /**
  * Represents the http servers and filters of a container cluster.
  *
  * @author Tony Vaagenes
+ * @author bjorncs
  */
 public class Http extends AbstractConfigProducer<AbstractConfigProducer<?>> implements ServerConfig.Producer {
 
-    private FilterChains filterChains;
-    private JettyHttpServer httpServer;
-    private List<Binding> bindings;
-    private final Optional<AccessControl> accessControl;
-
-    public Http(List<Binding> bindings) {
-        this(bindings, null);
-    }
+    private final Object monitor = new Object();
 
-    public Http(List<Binding> bindings, AccessControl accessControl) {
-        super( "http");
-        this.bindings = Collections.unmodifiableList(bindings);
-        this.accessControl = Optional.ofNullable(accessControl);
-    }
+    private final FilterChains filterChains;
+    private final List<Binding> bindings = new CopyOnWriteArrayList<>();
+    private JettyHttpServer httpServer;
+    private AccessControl accessControl;
 
-    public void setFilterChains(FilterChains filterChains) {
-        this.filterChains = filterChains;
+    public Http(FilterChains chains) {
+        super("http");
+        this.filterChains = chains;
     }
 
-    public void setBindings(List<Binding> bindings) {
-        this.bindings = Collections.unmodifiableList(bindings);
+    public void setAccessControl(AccessControl accessControl) {
+        synchronized (monitor) {
+            if (this.accessControl != null) throw new IllegalStateException("Access control already assigned");
+            this.accessControl = accessControl;
+        }
     }
 
     public FilterChains getFilterChains() {
-        return filterChains;
+        synchronized (monitor) {
+            return filterChains;
+        }
     }
 
-    public JettyHttpServer getHttpServer() {
-        return httpServer;
+    public Optional<JettyHttpServer> getHttpServer() {
+        synchronized (monitor) {
+            return Optional.ofNullable(httpServer);
+        }
     }
 
     public void setHttpServer(JettyHttpServer newServer) {
-        JettyHttpServer oldServer = this.httpServer;
-        this.httpServer = newServer;
-
-        if (oldServer == null && newServer != null) {
-            addChild(newServer);
-        } else if (newServer == null && oldServer != null) {
-            removeChild(oldServer);
-        } else if (newServer == null && oldServer == null) {
-            //do nothing
-        } else {
-            //none of them are null
-            removeChild(oldServer);
-            addChild(newServer);
+        synchronized (monitor) {
+            JettyHttpServer oldServer = this.httpServer;
+            this.httpServer = newServer;
+
+            if (oldServer == null && newServer != null) {
+                addChild(newServer);
+            } else if (newServer == null && oldServer != null) {
+                removeChild(oldServer);
+            } else if (newServer == null && oldServer == null) {
+                //do nothing
+            } else {
+                //none of them are null
+                removeChild(oldServer);
+                addChild(newServer);
+            }
         }
     }
 
     public void removeAllServers() {
-        setHttpServer(null);
+        synchronized (monitor) {
+            setHttpServer(null);
+        }
     }
 
     public List<Binding> getBindings() {
-        return bindings;
+        synchronized (monitor) {
+            return bindings;
+        }
     }
 
     public Optional<AccessControl> getAccessControl() {
-        return accessControl;
+        synchronized (monitor) {
+            return Optional.ofNullable(accessControl);
+        }
     }
 
     @Override
     public void getConfig(ServerConfig.Builder builder) {
-        for (Binding binding : bindings) {
-            builder.filter(new ServerConfig.Filter.Builder()
-                                   .id(binding.filterId().stringValue())
-                                   .binding(binding.binding()));
+        synchronized (monitor) {
+            for (Binding binding : bindings) {
+                builder.filter(new ServerConfig.Filter.Builder()
+                                       .id(binding.filterId().stringValue())
+                                       .binding(binding.binding()));
+            }
         }
     }
 
     @Override
     public void validate() {
-        validate(bindings);
-    }
-
-    void validate(Collection<Binding> bindings) {
-        if (bindings.isEmpty()) return;
+        synchronized (monitor) {
+            if (((Collection<Binding>) bindings).isEmpty()) return;
 
-        if (filterChains == null)
-            throw new IllegalArgumentException("Null FilterChains are not allowed when there are filter bindings");
+            if (filterChains == null)
+                throw new IllegalArgumentException("Null FilterChains are not allowed when there are filter bindings");
 
-        ComponentRegistry<ChainedComponent<?>> filters = filterChains.componentsRegistry();
-        ComponentRegistry<Chain<Filter>> chains = filterChains.allChains();
+            ComponentRegistry<ChainedComponent<?>> filters = filterChains.componentsRegistry();
+            ComponentRegistry<Chain<Filter>> chains = filterChains.allChains();
 
-        for (Binding binding: bindings) {
-            if (filters.getComponent(binding.filterId()) == null && chains.getComponent(binding.filterId()) == null)
-                throw new RuntimeException("Can't find filter " + binding.filterId() + " for binding " + binding.binding());
+            for (Binding binding: bindings) {
+                if (filters.getComponent(binding.filterId()) == null && chains.getComponent(binding.filterId()) == null)
+                    throw new RuntimeException("Can't find filter " + binding.filterId() + " for binding " + binding.binding());
+            }
         }
     }
-
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
index 61a588fb716..bfde9b9add1 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
@@ -54,11 +54,10 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http>
             filterChains = new FilterChainsBuilder().newChainsInstance(ancestor);
         }
 
-        Http http = new Http(bindings, accessControl);
-        http.setFilterChains(filterChains);
-
-        buildHttpServers(deployState, ancestor, http, spec);
-
+        Http http = new Http(filterChains);
+        http.getBindings().addAll(bindings);
+        http.setAccessControl(accessControl);
+        http.setHttpServer(new JettyHttpServerBuilder().build(deployState, ancestor, spec));
         return http;
     }
 
@@ -131,10 +130,6 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http>
         return result;
     }
 
-    private void buildHttpServers(DeployState deployState, AbstractConfigProducer ancestor, Http http, Element spec) {
-        http.setHttpServer(new JettyHttpServerBuilder().build(deployState, ancestor, spec));
-    }
-
     static int readPort(ModelElement spec, boolean isHosted, DeployLogger logger) {
         Integer port = spec.integerAttribute("port");
         if (port == null)
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
index c1f793e255d..a67c98bca49 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
@@ -330,7 +330,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
     }
 
     private void addAdditionalHostedConnector(DeployState deployState, ApplicationContainerCluster cluster) {
-        JettyHttpServer server = cluster.getHttp().getHttpServer();
+        JettyHttpServer server = cluster.getHttp().getHttpServer().get();
         String serverName = server.getComponentId().getName();
 
         String proxyProtocol = deployState.getProperties().proxyProtocol();
@@ -363,7 +363,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
                     .orElseGet(() -> createHostedImplicitHttpWithoutAccessControl(cluster));
             cluster.setHttp(http);
         }
-        if(cluster.getHttp().getHttpServer() == null) {
+        if(cluster.getHttp().getHttpServer().isEmpty()) {
             JettyHttpServer defaultHttpServer = new JettyHttpServer(new ComponentId("DefaultHttpServer"));
             cluster.getHttp().setHttpServer(defaultHttpServer);
             defaultHttpServer.addConnector(new ConnectorFactory("SearchServer", Defaults.getDefaults().vespaWebServicePort()));
@@ -378,17 +378,16 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
                         .readEnabled(false)
                         .writeEnabled(false)
                         .build();
-        Http http = new Http(accessControl.getBindings(), accessControl);
         FilterChains filterChains = new FilterChains(cluster);
         filterChains.add(new Chain<>(FilterChains.emptyChainSpec(ACCESS_CONTROL_CHAIN_ID)));
-        http.setFilterChains(filterChains);
+        Http http = new Http(filterChains);
+        http.setAccessControl(accessControl);
+        http.getBindings().addAll(accessControl.getBindings());
         return http;
     }
 
     private static Http createHostedImplicitHttpWithoutAccessControl(ApplicationContainerCluster cluster) {
-        Http http = new Http(Collections.emptyList());
-        http.setFilterChains(new FilterChains(cluster));
-        return http;
+        return new Http(new FilterChains(cluster));
     }
 
     private Http buildHttp(DeployState deployState, ApplicationContainerCluster cluster, Element httpElement) {
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
index 73db6e35428..12365fb773c 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
@@ -811,7 +811,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase {
         ApplicationContainer container = (ApplicationContainer)root.getProducer("container/container.0");
 
         // Verify that there are two connectors
-        List<ConnectorFactory> connectorFactories = container.getHttp().getHttpServer().getConnectorFactories();
+        List<ConnectorFactory> connectorFactories = container.getHttp().getHttpServer().get().getConnectorFactories();
         assertEquals(2, connectorFactories.size());
         List<Integer> ports = connectorFactories.stream()
                 .map(ConnectorFactory::getListenPort)