Merge pull request #12783 from vespa-engine/bjorncs/max-connection-life

Bjorncs/max connection life

8 files changed, 50 insertions, 43 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/ssl/HostedSslConnectorFactory.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/ssl/HostedSslConnectorFactory.java
index f61618c789b..fb8e9dffbbb 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/ssl/HostedSslConnectorFactory.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/ssl/HostedSslConnectorFactory.java
@@ -7,6 +7,7 @@ import com.yahoo.jdisc.http.ConnectorConfig.Ssl.ClientAuth;
 import com.yahoo.vespa.model.container.component.SimpleComponent;
 import com.yahoo.vespa.model.container.http.ConnectorFactory;
 
+import java.time.Duration;
 import java.util.List;
 
 /**
@@ -67,10 +68,13 @@ public class HostedSslConnectorFactory extends ConnectorFactory {
     @Override
     public void getConfig(ConnectorConfig.Builder connectorBuilder) {
         super.getConfig(connectorBuilder);
-        connectorBuilder.tlsClientAuthEnforcer(new ConnectorConfig.TlsClientAuthEnforcer.Builder()
-                                                       .pathWhitelist(INSECURE_WHITELISTED_PATHS)
-                                                       .enable(enforceClientAuth));
-        connectorBuilder.proxyProtocol(configureProxyProtocol());
+        connectorBuilder
+                .tlsClientAuthEnforcer(new ConnectorConfig.TlsClientAuthEnforcer.Builder()
+                                               .pathWhitelist(INSECURE_WHITELISTED_PATHS)
+                                               .enable(enforceClientAuth))
+                .proxyProtocol(configureProxyProtocol())
+                .idleTimeout(Duration.ofMinutes(3).toSeconds())
+                .maxConnectionLife(Duration.ofMinutes(10).toSeconds());
     }
 
     private ConnectorConfig.ProxyProtocol.Builder configureProxyProtocol() {
diff --git a/jdisc_http_service/abi-spec.json b/jdisc_http_service/abi-spec.json
index 483565cc69f..c5a0a676a70 100644
--- a/jdisc_http_service/abi-spec.json
+++ b/jdisc_http_service/abi-spec.json
@@ -43,6 +43,8 @@
       "public com.yahoo.jdisc.http.ConnectorConfig$Builder healthCheckProxy(com.yahoo.jdisc.http.ConnectorConfig$HealthCheckProxy$Builder)",
       "public com.yahoo.jdisc.http.ConnectorConfig$Builder proxyProtocol(com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol$Builder)",
       "public com.yahoo.jdisc.http.ConnectorConfig$Builder secureRedirect(com.yahoo.jdisc.http.ConnectorConfig$SecureRedirect$Builder)",
+      "public com.yahoo.jdisc.http.ConnectorConfig$Builder maxRequestsPerConnection(int)",
+      "public com.yahoo.jdisc.http.ConnectorConfig$Builder maxConnectionLife(double)",
       "public final boolean dispatchGetConfig(com.yahoo.config.ConfigInstance$Producer)",
       "public final java.lang.String getDefMd5()",
       "public final java.lang.String getDefName()",
@@ -357,7 +359,9 @@
       "public com.yahoo.jdisc.http.ConnectorConfig$TlsClientAuthEnforcer tlsClientAuthEnforcer()",
       "public com.yahoo.jdisc.http.ConnectorConfig$HealthCheckProxy healthCheckProxy()",
       "public com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol proxyProtocol()",
-      "public com.yahoo.jdisc.http.ConnectorConfig$SecureRedirect secureRedirect()"
+      "public com.yahoo.jdisc.http.ConnectorConfig$SecureRedirect secureRedirect()",
+      "public int maxRequestsPerConnection()",
+      "public double maxConnectionLife()"
     ],
     "fields": [
       "public static final java.lang.String CONFIG_DEF_MD5",
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java
index 71dcb7d0682..b9d686c1d6b 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java
@@ -10,6 +10,7 @@ import com.yahoo.jdisc.handler.BindingNotFoundException;
 import com.yahoo.jdisc.handler.ContentChannel;
 import com.yahoo.jdisc.handler.OverloadException;
 import com.yahoo.jdisc.handler.RequestHandler;
+import com.yahoo.jdisc.http.ConnectorConfig;
 import com.yahoo.jdisc.http.HttpHeaders;
 import com.yahoo.jdisc.http.HttpRequest;
 import org.eclipse.jetty.io.EofException;
@@ -22,6 +23,7 @@ import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
+import java.time.Instant;
 import java.util.Arrays;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
@@ -34,6 +36,7 @@ import java.util.logging.Logger;
 import static com.yahoo.jdisc.http.HttpHeaders.Values.APPLICATION_X_WWW_FORM_URLENCODED;
 import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnection;
 import static com.yahoo.jdisc.http.server.jetty.Exceptions.throwUnchecked;
+import static com.yahoo.jdisc.http.server.jetty.JDiscHttpServlet.getConnector;
 
 /**
  * @author Simon Thoresen Hult
@@ -64,14 +67,13 @@ class HttpRequestDispatch {
 
         this.jettyRequest = (Request) servletRequest;
         this.metricReporter = new MetricReporter(jDiscContext.metric, metricContext, jettyRequest.getTimeStamp());
-        honourMaxKeepAliveRequests();
         this.servletResponseController = new ServletResponseController(
                 servletRequest,
                 servletResponse,
                 jDiscContext.janitor,
                 metricReporter,
                 jDiscContext.developerMode());
-
+        markConnectionAsNonPersistentIfThresholdReached(servletRequest);
         this.async = servletRequest.startAsync();
         async.setTimeout(0);
         metricReporter.uriLength(jettyRequest.getOriginalURI().length());
@@ -102,15 +104,6 @@ class HttpRequestDispatch {
         }
     }
 
-    private void honourMaxKeepAliveRequests() {
-        if (jDiscContext.serverConfig.maxKeepAliveRequests() > 0) {
-            HttpConnection connection = getConnection(jettyRequest);
-            if (connection.getMessagesIn() >= jDiscContext.serverConfig.maxKeepAliveRequests()) {
-                connection.getGenerator().setPersistent(false);
-            }
-        }
-    }
-
     private BiConsumer<Void, Throwable> completeRequestCallback;
     {
         AtomicBoolean completeRequestCalled = new AtomicBoolean(false);
@@ -151,6 +144,25 @@ class HttpRequestDispatch {
         };
     }
 
+    private static void markConnectionAsNonPersistentIfThresholdReached(HttpServletRequest request) {
+        ConnectorConfig connectorConfig = getConnector(request).connectorConfig();
+        int maxRequestsPerConnection = connectorConfig.maxRequestsPerConnection();
+        if (maxRequestsPerConnection > 0) {
+            HttpConnection connection = getConnection(request);
+            if (connection.getMessagesIn() >= maxRequestsPerConnection) {
+                connection.getGenerator().setPersistent(false);
+            }
+        }
+        double maxConnectionLifeInSeconds = connectorConfig.maxConnectionLife();
+        if (maxConnectionLifeInSeconds > 0) {
+            HttpConnection connection = getConnection(request);
+            Instant expireAt = Instant.ofEpochMilli((long)(connection.getCreatedTimeStamp() + maxConnectionLifeInSeconds * 1000));
+            if (Instant.now().isAfter(expireAt)) {
+                connection.getGenerator().setPersistent(false);
+            }
+        }
+    }
+
     @SafeVarargs
     @SuppressWarnings("varargs")
     private static boolean isErrorOfType(Throwable throwable, Class<? extends Throwable>... handledTypes) {
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java
index cf66af31a79..5cbe7320f0e 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java
@@ -100,6 +100,8 @@ class JDiscHttpServlet extends HttpServlet {
         }
     }
 
+
+
     static JDiscServerConnector getConnector(HttpServletRequest request) {
         return (JDiscServerConnector)getConnection(request).getConnector();
     }
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 1e4a2082094..c5f42ff9cc5 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
@@ -34,9 +34,6 @@ import org.eclipse.jetty.servlet.ServletHolder;
 import org.eclipse.jetty.util.log.JavaUtilLog;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.thread.QueuedThreadPool;
-import org.osgi.framework.BundleContext;
-import org.osgi.framework.InvalidSyntaxException;
-import org.osgi.framework.ServiceReference;
 
 import javax.management.remote.JMXServiceURL;
 import javax.servlet.DispatcherType;
@@ -44,10 +41,8 @@ import java.io.IOException;
 import java.lang.management.ManagementFactory;
 import java.net.BindException;
 import java.net.MalformedURLException;
-import java.nio.channels.ServerSocketChannel;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.List;
@@ -285,25 +280,6 @@ public class JettyHttpServer extends AbstractServerProvider {
         return ports.stream().map(Object::toString).collect(Collectors.joining(":"));
     }
 
-    private ServerSocketChannel getChannelFromServiceLayer(int listenPort, BundleContext bundleContext) {
-        log.log(Level.FINE, "Retrieving channel for port " + listenPort + " from " + bundleContext.getClass().getName());
-        Collection<ServiceReference<ServerSocketChannel>> refs;
-        final String filter = "(port=" + listenPort + ")";
-        try {
-            refs = bundleContext.getServiceReferences(ServerSocketChannel.class, filter);
-        } catch (InvalidSyntaxException e) {
-            throw new IllegalStateException("OSGi framework rejected filter " + filter, e);
-        }
-        if (refs.isEmpty()) {
-            return null;
-        }
-        if (refs.size() != 1) {
-            throw new IllegalStateException("Got more than one service reference for " + ServerSocketChannel.class + " port " + listenPort + ".");
-        }
-        ServiceReference<ServerSocketChannel> ref = refs.iterator().next();
-        return bundleContext.getService(ref);
-    }
-
     private static ExecutorService newJanitor(ThreadFactory factory) {
         int threadPoolSize = Runtime.getRuntime().availableProcessors();
         log.info("Creating janitor executor with " + threadPoolSize + " threads");
diff --git a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.connector.def b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.connector.def
index 93378975609..fa7ed6657d9 100644
--- a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.connector.def
+++ b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.connector.def
@@ -112,3 +112,9 @@ secureRedirect.enabled         bool    default=false
 
 # Target port for redirect
 secureRedirect.port            int     default=443
+
+# Maximum number of request per connection before server marks connections as non-persistent. Set to '0' to disable.
+maxRequestsPerConnection       int     default=0
+
+# Maximum number of seconds a connection can live before it's marked as non-persistent. Set to '0' to disable.
+maxConnectionLife              double  default=0.0
diff --git a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.server.def b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.server.def
index 0836a080e1f..33f82963243 100644
--- a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.server.def
+++ b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.server.def
@@ -7,13 +7,16 @@ developerMode                         bool   default=false
 # The gzip compression level to use, if compression is enabled in a request.
 responseCompressionLevel              int    default=6
 
-# Whether to enable HTTP keep-alive for requests that support this.
+# DEPRECATED - Ignored, no longer in use.
 httpKeepAliveEnabled                  bool   default=true
+# TODO Vespa 8 Remove httpKeepAliveEnabled
 
 # Maximum number of request per http connection before server will hangup.
 # Naming taken from apache http server.
 # 0 means never hangup.
+# DEPRECATED - Ignored, no longer in use. Use similar parameter in connector config instead.
 maxKeepAliveRequests                  int    default=0
+# TODO Vespa 8 Remove maxKeepAliveRequests
 
 # Whether the request body of POSTed forms should be removed (form parameters are available as request parameters).
 removeRawPostBodyForWwwUrlEncodedPost bool   default=false
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java
index 6ace9699b42..f2f3fb0ef11 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java
@@ -484,8 +484,8 @@ public class HttpServerTest {
     public void requireThatConnectionIsClosedAfterXRequests() throws Exception {
         final int MAX_KEEPALIVE_REQUESTS = 100;
         final TestDriver driver = TestDrivers.newConfiguredInstance(new EchoRequestHandler(),
-                new ServerConfig.Builder().maxKeepAliveRequests(MAX_KEEPALIVE_REQUESTS),
-                new ConnectorConfig.Builder());
+                new ServerConfig.Builder(),
+                new ConnectorConfig.Builder().maxRequestsPerConnection(MAX_KEEPALIVE_REQUESTS));
         for (int i = 0; i < MAX_KEEPALIVE_REQUESTS - 1; i++) {
             driver.client().get("/status.html")
                     .expectStatusCode(is(OK))