diff options
author | Bjørn Christian Seime <bjorncs@verizonmedia.com> | 2021-08-09 17:43:56 +0200 |
---|---|---|
committer | Bjørn Christian Seime <bjorncs@verizonmedia.com> | 2021-08-09 17:43:56 +0200 |
commit | 14ac5b3dbcf5a108b201bb7a09bc09e81064efe9 (patch) | |
tree | d5e9f295377814d422d5e3bc3cb140d617ed3465 /container-core | |
parent | 904dfd949a04be4dcbd9f4dfea0f6b87da84f942 (diff) |
Respect thresholds for max requests/time for HTTP/2 connections
Diffstat (limited to 'container-core')
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()); } |