[tomcat] branch 9.0.x updated: Fix BZ 64467. Improve performance of closing idle streams

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[tomcat] branch 9.0.x updated: Fix BZ 64467. Improve performance of closing idle streams

markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/9.0.x by this push:
     new 9a02316  Fix BZ 64467. Improve performance of closing idle streams
9a02316 is described below

commit 9a0231683a77e2957cea0fdee88b193b30b0c976
Author: Mark Thomas <[hidden email]>
AuthorDate: Fri May 22 11:27:49 2020 +0100

    Fix BZ 64467. Improve performance of closing idle streams
---
 .../apache/coyote/http2/Http2UpgradeHandler.java   | 10 +++----
 .../apache/coyote/http2/TestHttp2Section_5_1.java  | 31 +++++++++++++++++++---
 webapps/docs/changelog.xml                         |  4 +++
 3 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index bd83694..f0d5f27 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1473,11 +1473,11 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
     }
 
 
-    private void closeIdleStreams(int newMaxActiveRemoteStreamId) throws Http2Exception {
-        for (int i = maxActiveRemoteStreamId + 2; i < newMaxActiveRemoteStreamId; i += 2) {
-            Stream stream = getStream(i, false);
-            if (stream != null) {
-                stream.closeIfIdle();
+    private void closeIdleStreams(int newMaxActiveRemoteStreamId) {
+        for (Entry<Integer,Stream> entry : streams.entrySet()) {
+            if (entry.getKey().intValue() > maxActiveRemoteStreamId &&
+                    entry.getKey().intValue() < newMaxActiveRemoteStreamId) {
+                entry.getValue().closeIfIdle();
             }
         }
         maxActiveRemoteStreamId = newMaxActiveRemoteStreamId;
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
index 2a46681..f878653 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
@@ -147,21 +147,44 @@ public class TestHttp2Section_5_1 extends Http2TestBase {
 
     @Test
     public void testImplicitClose() throws Exception {
+        doTestImplicitClose(5);
+    }
+
+
+    // https://bz.apache.org/bugzilla/show_bug.cgi?id=64467
+    @Test
+    public void testImplicitCloseLargeId() throws Exception {
+        doTestImplicitClose(Integer.MAX_VALUE - 8);
+    }
+
+
+    private void doTestImplicitClose(int lastStreamId) throws Exception {
+
+        long startFirst = System.nanoTime();
         http2Connect();
+        long durationFirst = System.nanoTime() - startFirst;
 
         sendPriority(3, 0, 16);
-        sendPriority(5, 0, 16);
+        sendPriority(lastStreamId, 0, 16);
 
-        sendSimpleGetRequest(5);
+        long startSecond = System.nanoTime();
+        sendSimpleGetRequest(lastStreamId);
         readSimpleGetResponse();
-        Assert.assertEquals(getSimpleResponseTrace(5), output.getTrace());
+        long durationSecond = System.nanoTime() - startSecond;
+
+        Assert.assertEquals(getSimpleResponseTrace(lastStreamId), output.getTrace());
         output.clearTrace();
 
+        // Allow second request to take up to 5 times first request or up to 1 second - whichever is the larger - mainly
+        // to allow for CI systems under load that can exhibit significant timing variation.
+        Assert.assertTrue("First request took [" + durationFirst/1000000 + "ms], second request took [" +
+                durationSecond/1000000 + "ms]", durationSecond < 1000000000 || durationSecond < durationFirst * 3);
+
         // Should trigger an error since stream 3 should have been implicitly
         // closed.
         sendSimpleGetRequest(3);
 
-        handleGoAwayResponse(5);
+        handleGoAwayResponse(lastStreamId);
     }
 
 
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index d7a6873..db6da26 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -71,6 +71,10 @@
       <update>
         Add support for ALPN on recent OpenJDK 8 releases. (remm)
       </update>
+      <fix>
+        <bug>64467</bug>: Improve performance of closing idle HTTP/2 streams.
+        (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="WebSocket">


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]