From 5d7f2eac857cc75757cfc58d003fbf17a23c2720 Mon Sep 17 00:00:00 2001
From: Mark Thomas <markt@apache.org>
Date: Wed, 7 Aug 2019 17:02:37 +0100
Subject: [PATCH 3/5] Improve HTTP/2 connection timeout handling

---
 .../http2/Http2AsyncUpgradeHandler.java       |  6 +-
 .../coyote/http2/Http2UpgradeHandler.java     | 93 ++++++++++++++-----
 2 files changed, 73 insertions(+), 26 deletions(-)

diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
index ba49986b5b..107d4bedd2 100644
--- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
@@ -191,7 +191,7 @@ public class Http2AsyncUpgradeHandler extends Http2UpgradeHandler {
             header[4] = FLAG_END_OF_STREAM;
             stream.sentEndOfStream();
             if (!stream.isActive()) {
-                activeRemoteStreamCount.decrementAndGet();
+                setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
             }
         }
         if (writeable) {
@@ -297,7 +297,7 @@ public class Http2AsyncUpgradeHandler extends Http2UpgradeHandler {
                 header[4] = FLAG_END_OF_STREAM;
                 sendfile.stream.sentEndOfStream();
                 if (!sendfile.stream.isActive()) {
-                    activeRemoteStreamCount.decrementAndGet();
+                    setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
                 }
             }
             if (writeable) {
@@ -358,7 +358,7 @@ public class Http2AsyncUpgradeHandler extends Http2UpgradeHandler {
                 header[4] = FLAG_END_OF_STREAM;
                 sendfile.stream.sentEndOfStream();
                 if (!sendfile.stream.isActive()) {
-                    activeRemoteStreamCount.decrementAndGet();
+                    setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
                 }
             }
             if (writeable) {
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 2330d12e09..9c18bf0ca8 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -133,6 +133,9 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
     // Tracking for when the connection is blocked (windowSize < 1)
     private final Map<AbstractStream,int[]> backLogStreams = new ConcurrentHashMap<>();
     private long backLogSize = 0;
+    // The time at which the connection will timeout unless data arrives before
+    // then. -1 means no timeout.
+    private volatile long connectionTimeout = -1;
 
     // Stream concurrency control
     private AtomicInteger streamConcurrency = null;
@@ -313,8 +316,10 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
             case OPEN_READ:
                 try {
                     // There is data to read so use the read timeout while
-                    // reading frames.
+                    // reading frames ...
                     socketWrapper.setReadTimeout(protocol.getReadTimeout());
+                    // ... and disable the connection timeout
+                    setConnectionTimeout(-1);
                     while (true) {
                         try {
                             if (!parser.readFrame(false)) {
@@ -330,23 +335,22 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
                                 stream.close(se);
                             }
                         }
+                        if (overheadCount.get() > 0) {
+                            throw new ConnectionException(
+                                    sm.getString("upgradeHandler.tooMuchOverhead", connectionId),
+                                    Http2Error.ENHANCE_YOUR_CALM);
+                        }
                     }
 
-                    if (overheadCount.get() > 0) {
-                        throw new ConnectionException(
-                                sm.getString("upgradeHandler.tooMuchOverhead", connectionId),
-                                Http2Error.ENHANCE_YOUR_CALM);
-                    }
+                    // Need to know the correct timeout before starting the read
+                    // but that may not be known at this time if one or more
+                    // requests are currently being processed so don't set a
+                    // timeout for the socket...
+                    socketWrapper.setReadTimeout(-1);
+
+                    // ...set a timeout on the connection
+                    setConnectionTimeoutForStreamCount(activeRemoteStreamCount.get());
 
-                    if (activeRemoteStreamCount.get() == 0) {
-                        // No streams currently active. Use the keep-alive
-                        // timeout for the connection.
-                        socketWrapper.setReadTimeout(protocol.getKeepAliveTimeout());
-                    } else {
-                        // Streams currently active. Individual streams have
-                        // timeouts so keep the connection open.
-                        socketWrapper.setReadTimeout(-1);
-                    }
                 } catch (Http2Exception ce) {
                     // Really ConnectionException
                     if (log.isDebugEnabled()) {
@@ -367,9 +371,12 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
                 result = SocketState.UPGRADED;
                 break;
 
+            case TIMEOUT:
+                closeConnection(null);
+                break;
+
             case DISCONNECT:
             case ERROR:
-            case TIMEOUT:
             case STOP:
                 close();
                 break;
@@ -388,9 +395,41 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
     }
 
 
+    /*
+     * Sets the connection timeout based on the current number of active
+     * streams.
+     */
+    protected void setConnectionTimeoutForStreamCount(int streamCount) {
+        if (streamCount == 0) {
+            // No streams currently active. Use the keep-alive
+            // timeout for the connection.
+            long keepAliveTimeout = protocol.getKeepAliveTimeout();
+            if (keepAliveTimeout == -1) {
+                setConnectionTimeout(-1);
+            } else {
+                setConnectionTimeout(System.currentTimeMillis() + keepAliveTimeout);
+            }
+        } else {
+            // Streams currently active. Individual streams have
+            // timeouts so keep the connection open.
+            setConnectionTimeout(-1);
+        }
+    }
+
+
+    private void setConnectionTimeout(long connectionTimeout) {
+        this.connectionTimeout = connectionTimeout;
+    }
+
+
     @Override
     public void timeoutAsync(long now) {
-        // TODO: Implement improved connection timeouts
+        long connectionTimeout = this.connectionTimeout;
+        if (now == -1 || connectionTimeout > -1 && now > connectionTimeout) {
+            // Have to dispatch as this will be executed from a non-container
+            // thread.
+            socketWrapper.processSocket(SocketEvent.TIMEOUT, true);
+        }
     }
 
 
@@ -499,9 +538,17 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
 
 
     void closeConnection(Http2Exception ce) {
+        long code;
+        byte[] msg;
+        if (ce == null) {
+            code = Http2Error.NO_ERROR.getCode();
+            msg = null;
+        } else {
+            code = ce.getError().getCode();
+            msg = ce.getMessage().getBytes(StandardCharsets.UTF_8);
+        }
         try {
-            writeGoAwayFrame(maxProcessedStreamId, ce.getError().getCode(),
-                    ce.getMessage().getBytes(StandardCharsets.UTF_8));
+            writeGoAwayFrame(maxProcessedStreamId, code, msg);
         } catch (IOException ioe) {
             // Ignore. GOAWAY is sent on a best efforts basis and the original
             // error has already been logged.
@@ -665,7 +712,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
             header[4] = FLAG_END_OF_STREAM;
             stream.sentEndOfStream();
             if (!stream.isActive()) {
-                activeRemoteStreamCount.decrementAndGet();
+                setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
             }
         }
         if (writeable) {
@@ -1182,7 +1229,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
         if (localSettings.getMaxConcurrentStreams() < activeRemoteStreamCount.incrementAndGet()) {
             // If there are too many open streams, simply ignore the push
             // request.
-            activeRemoteStreamCount.decrementAndGet();
+            setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
             return;
         }
 
@@ -1301,7 +1348,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
         if (stream != null) {
             stream.receivedEndOfStream();
             if (!stream.isActive()) {
-                activeRemoteStreamCount.decrementAndGet();
+                setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
             }
         }
     }
@@ -1340,7 +1387,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
             stream.receivedStartOfHeaders(headersEndStream);
             closeIdleStreams(streamId);
             if (localSettings.getMaxConcurrentStreams() < activeRemoteStreamCount.incrementAndGet()) {
-                activeRemoteStreamCount.decrementAndGet();
+                setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
                 throw new StreamException(sm.getString("upgradeHandler.tooManyRemoteStreams",
                         Long.toString(localSettings.getMaxConcurrentStreams())),
                         Http2Error.REFUSED_STREAM, streamId);
-- 
2.23.0