Respect thresholds for max requests/time for HTTP/2 connections

3 files changed, 68 insertions, 26 deletions

diff --git a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java
index ba292062197..6b206116de8 100644
--- a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java
+++ b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java
@@ -13,8 +13,13 @@ 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.http2.ErrorCode;
+import org.eclipse.jetty.http2.server.HTTP2ServerConnection;
+import org.eclipse.jetty.io.Connection;
 import org.eclipse.jetty.io.EofException;
+import org.eclipse.jetty.server.HttpConnection;
 import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.util.Callback;
 
 import javax.servlet.AsyncContext;
 import javax.servlet.ServletInputStream;
@@ -35,7 +40,6 @@ import java.util.logging.Logger;
 
 import static com.yahoo.jdisc.http.HttpHeaders.Values.APPLICATION_X_WWW_FORM_URLENCODED;
 import static com.yahoo.jdisc.http.server.jetty.RequestUtils.getConnector;
-import static com.yahoo.jdisc.http.server.jetty.RequestUtils.getHttp1Connection;
 import static com.yahoo.yolean.Exceptions.throwUnchecked;
 
 /**
@@ -72,7 +76,7 @@ class HttpRequestDispatch {
                                                                        jDiscContext.janitor,
                                                                        metricReporter,
                                                                        jDiscContext.developerMode());
-        markHttp1ConnectionAsNonPersistentIfThresholdReached(jettyRequest);
+        shutdownConnectionGracefullyIfThresholdReached(jettyRequest);
         this.async = servletRequest.startAsync();
         async.setTimeout(0);
         metricReporter.uriLength(jettyRequest.getOriginalURI().length());
@@ -143,24 +147,34 @@ class HttpRequestDispatch {
         };
     }
 
-    private static void markHttp1ConnectionAsNonPersistentIfThresholdReached(Request request) {
+    private static void shutdownConnectionGracefullyIfThresholdReached(Request request) {
         ConnectorConfig connectorConfig = getConnector(request).connectorConfig();
         int maxRequestsPerConnection = connectorConfig.maxRequestsPerConnection();
+        Connection connection = RequestUtils.getConnection(request);
         if (maxRequestsPerConnection > 0) {
-            getHttp1Connection(request).ifPresent(connection -> {
-                if (connection.getMessagesIn() >= maxRequestsPerConnection) {
-                    connection.getGenerator().setPersistent(false);
-                }
-            });
+            if (connection.getMessagesIn() >= maxRequestsPerConnection) {
+                gracefulShutdown(connection, "max-req-per-conn-exceeded");
+            }
         }
         double maxConnectionLifeInSeconds = connectorConfig.maxConnectionLife();
         if (maxConnectionLifeInSeconds > 0) {
-            getHttp1Connection(request).ifPresent(connection -> {
-                Instant expireAt = Instant.ofEpochMilli((long) (connection.getCreatedTimeStamp() + maxConnectionLifeInSeconds * 1000));
-                if (Instant.now().isAfter(expireAt)) {
-                    connection.getGenerator().setPersistent(false);
-                }
-            });
+            long createdAt = connection.getCreatedTimeStamp();
+            Instant expiredAt = Instant.ofEpochMilli((long) (createdAt + maxConnectionLifeInSeconds * 1000));
+            boolean isExpired = Instant.now().isAfter(expiredAt);
+            if (isExpired) {
+                gracefulShutdown(connection, "max-conn-life-exceeded");
+            }
+        }
+    }
+
+    private static void gracefulShutdown(Connection connection, String reason) {
+        if (connection instanceof HttpConnection) {
+            HttpConnection http1 = (HttpConnection) connection;
+            http1.getGenerator().setPersistent(false);
+        } else if (connection instanceof HTTP2ServerConnection) {
+            HTTP2ServerConnection http2 = (HTTP2ServerConnection) connection;
+            // Signal Jetty to do a graceful connection shutdown with GOAWAY frame
+            http2.getSession().close(ErrorCode.NO_ERROR.code, reason, Callback.NOOP);
         }
     }
 
diff --git a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/RequestUtils.java b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/RequestUtils.java
index b248f55a3df..73bac077bc5 100644
--- a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/RequestUtils.java
+++ b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/RequestUtils.java
@@ -7,7 +7,6 @@ import org.eclipse.jetty.server.HttpConnection;
 import org.eclipse.jetty.server.Request;
 
 import javax.servlet.http.HttpServletRequest;
-import java.util.Optional;
 
 /**
  * @author bjorncs
@@ -19,12 +18,6 @@ public class RequestUtils {
         return request.getHttpChannel().getConnection();
     }
 
-    public static Optional<HttpConnection> getHttp1Connection(Request request) {
-        Connection connection = getConnection(request);
-        if (connection instanceof HttpConnection) return Optional.of((HttpConnection) connection);
-        return Optional.empty();
-    }
-
     public static JDiscServerConnector getConnector(Request request) {
         return (JDiscServerConnector) request.getHttpChannel().getConnector();
     }
diff --git a/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java b/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java
index 86a3808becc..be96fc2332d 100644
--- a/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java
+++ b/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java
@@ -61,6 +61,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.UUID;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.regex.Pattern;
 
@@ -91,6 +92,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 import static org.mockito.Mockito.atLeast;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -456,11 +458,28 @@ public class HttpServerTest {
 
     @Test
     public void requireThatConnectionIsClosedAfterXRequests() throws Exception {
-        final int MAX_KEEPALIVE_REQUESTS = 100;
-        final JettyTestDriver driver = JettyTestDriver.newConfiguredInstance(new EchoRequestHandler(),
-                new ServerConfig.Builder(),
-                new ConnectorConfig.Builder().maxRequestsPerConnection(MAX_KEEPALIVE_REQUESTS));
-        for (int i = 0; i < MAX_KEEPALIVE_REQUESTS - 1; i++) {
+        final int MAX_REQUESTS = 10;
+        Path privateKeyFile = tmpFolder.newFile().toPath();
+        Path certificateFile = tmpFolder.newFile().toPath();
+        generatePrivateKeyAndCertificate(privateKeyFile, certificateFile);
+        ConnectorConfig.Builder connectorConfig = new ConnectorConfig.Builder()
+                .maxRequestsPerConnection(MAX_REQUESTS)
+                .ssl(new ConnectorConfig.Ssl.Builder()
+                        .enabled(true)
+                        .clientAuth(ConnectorConfig.Ssl.ClientAuth.Enum.NEED_AUTH)
+                        .privateKeyFile(privateKeyFile.toString())
+                        .certificateFile(certificateFile.toString())
+                        .caCertificateFile(certificateFile.toString()));
+        ServerConfig.Builder serverConfig = new ServerConfig.Builder()
+                .connectionLog(new ServerConfig.ConnectionLog.Builder().enabled(true));
+        JettyTestDriver driver = JettyTestDriver.newConfiguredInstance(
+                new EchoRequestHandler(),
+                serverConfig,
+                connectorConfig,
+                binder -> {});
+
+        // HTTP/1.1
+        for (int i = 0; i < MAX_REQUESTS - 1; i++) {
             driver.client().get("/status.html")
                     .expectStatusCode(is(OK))
                     .expectNoHeader(CONNECTION);
@@ -468,6 +487,22 @@ public class HttpServerTest {
         driver.client().get("/status.html")
                 .expectStatusCode(is(OK))
                 .expectHeader(CONNECTION, is(CLOSE));
+
+        // HTTP/2
+        try (CloseableHttpAsyncClient client = createHttp2Client(driver)) {
+            String uri = "https://localhost:" + driver.server().getListenPort() + "/status.html";
+            for (int i = 0; i < MAX_REQUESTS - 1; i++) {
+                SimpleHttpResponse response = client.execute(SimpleRequestBuilder.get(uri).build(), null).get();
+                assertEquals(OK, response.getCode());
+            }
+            try {
+                client.execute(SimpleRequestBuilder.get(uri).build(), null).get();
+                fail();
+            } catch (ExecutionException e) {
+                // Note: this is a weakness with Apache Http Client 5; the failed stream/request will not be retried on a new connection
+                assertEquals(e.getMessage(), "org.apache.hc.core5.http2.H2StreamResetException: Stream refused");
+            }
+        }
         assertTrue(driver.close());
     }