You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2022/11/18 11:41:19 UTC
[tomcat] branch 9.0.x updated: Fix bug with HTTP/2 stream reset and active connection count
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 97bfdd74aa Fix bug with HTTP/2 stream reset and active connection count
97bfdd74aa is described below
commit 97bfdd74aadbdd6eb9941b9e74b5a14777932377
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Fri Nov 18 11:07:17 2022 +0000
Fix bug with HTTP/2 stream reset and active connection count
When an HTTP/2 stream was reset, the current active stream count was not
reduced. If enough resets occurred on a connection, the current active
stream count limit was reached and no new streams could be created on
that connection.
---
.../coyote/http2/Http2AsyncUpgradeHandler.java | 4 ++
.../apache/coyote/http2/Http2UpgradeHandler.java | 4 ++
test/org/apache/coyote/http2/Http2TestBase.java | 22 +++++++---
.../coyote/http2/TestHttp2UpgradeHandler.java | 51 ++++++++++++++++++++++
webapps/docs/changelog.xml | 10 +++++
5 files changed, 85 insertions(+), 6 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
index 817402b606..69d2501158 100644
--- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
@@ -151,7 +151,11 @@ public class Http2AsyncUpgradeHandler extends Http2UpgradeHandler {
// order.
synchronized (sendResetLock) {
if (state != null) {
+ boolean active = state.isActive();
state.sendReset();
+ if (active) {
+ activeRemoteStreamCount.decrementAndGet();
+ }
}
socketWrapper.write(BlockingMode.SEMI_BLOCK, protocol.getWriteTimeout(),
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index a3c0c54b15..646b825f19 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -593,7 +593,11 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
// order.
synchronized (socketWrapper) {
if (state != null) {
+ boolean active = state.isActive();
state.sendReset();
+ if (active) {
+ activeRemoteStreamCount.decrementAndGet();
+ }
}
socketWrapper.write(true, rstFrame, 0, rstFrame.length);
socketWrapper.flush(true);
diff --git a/test/org/apache/coyote/http2/Http2TestBase.java b/test/org/apache/coyote/http2/Http2TestBase.java
index 66f178b208..fc3ad89f77 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -147,6 +147,11 @@ public abstract class Http2TestBase extends TomcatBaseTest {
protected void validateHttp2InitialResponse() throws Exception {
+ validateHttp2InitialResponse(200);
+ }
+
+ protected void validateHttp2InitialResponse(long maxConcurrentStreams) throws Exception {
+
// - 101 response acts as acknowledgement of the HTTP2-Settings header
// Need to read 5 frames
// - settings (server settings - must be first)
@@ -160,7 +165,7 @@ public abstract class Http2TestBase extends TomcatBaseTest {
parser.readFrame();
parser.readFrame();
- Assert.assertEquals("0-Settings-[3]-[200]\n" +
+ Assert.assertEquals("0-Settings-[3]-[" + maxConcurrentStreams + "]\n" +
"0-Settings-End\n" +
"0-Settings-Ack\n" +
"0-Ping-[0,0,0,0,0,0,0,1]\n" +
@@ -596,16 +601,21 @@ public abstract class Http2TestBase extends TomcatBaseTest {
}
protected void enableHttp2(long maxConcurrentStreams, boolean tls) {
+ enableHttp2(maxConcurrentStreams, tls, 10000, 10000, 25000, 5000, 5000);
+ }
+
+ protected void enableHttp2(long maxConcurrentStreams, boolean tls, long readTimeout, long writeTimeout,
+ long keepAliveTimeout, long streamReadTimout, long streamWriteTimeout) {
Tomcat tomcat = getTomcatInstance();
Connector connector = tomcat.getConnector();
Assert.assertTrue(connector.setProperty("useAsyncIO", Boolean.toString(useAsyncIO)));
http2Protocol = new UpgradableHttp2Protocol();
// Short timeouts for now. May need to increase these for CI systems.
- http2Protocol.setReadTimeout(10000);
- http2Protocol.setWriteTimeout(10000);
- http2Protocol.setKeepAliveTimeout(25000);
- http2Protocol.setStreamReadTimeout(5000);
- http2Protocol.setStreamWriteTimeout(5000);
+ http2Protocol.setReadTimeout(readTimeout);
+ http2Protocol.setWriteTimeout(writeTimeout);
+ http2Protocol.setKeepAliveTimeout(keepAliveTimeout);
+ http2Protocol.setStreamReadTimeout(streamReadTimout);
+ http2Protocol.setStreamWriteTimeout(streamWriteTimeout);
http2Protocol.setMaxConcurrentStreams(maxConcurrentStreams);
connector.addUpgradeProtocol(http2Protocol);
if (tls) {
diff --git a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
index f96895980a..b4565c7c72 100644
--- a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
+++ b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
@@ -186,4 +186,55 @@ public class TestHttp2UpgradeHandler extends Http2TestBase {
, output.getTrace());
}
}
+
+
+ @Test
+ public void testActiveConnectionCountAndClientTimeout() throws Exception {
+
+ enableHttp2(2, false, 10000, 10000, 4000, 2000, 2000);
+
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctxt = tomcat.addContext("", null);
+ Tomcat.addServlet(ctxt, "simple", new SimpleServlet());
+ ctxt.addServletMappingDecoded("/simple", "simple");
+
+ tomcat.start();
+
+ openClientConnection();
+ doHttpUpgrade();
+ sendClientPreface();
+ validateHttp2InitialResponse(2);
+
+ byte[] frameHeader = new byte[9];
+ ByteBuffer headersPayload = ByteBuffer.allocate(128);
+
+ byte[] dataFrameHeader = new byte[9];
+ ByteBuffer dataFramePayload = ByteBuffer.allocate(128);
+
+ // Should be able to make more than 2 requests even if they timeout
+ // since they should be removed from active connections once they
+ // timeout
+ for (int stream = 3; stream < 8; stream += 2) {
+ // Don't write the body. Allow the read to timeout.
+ buildPostRequest(frameHeader, headersPayload, false, dataFrameHeader, dataFramePayload, null, stream);
+ writeFrame(frameHeader, headersPayload);
+
+ // 500 response (triggered by IOException trying to read body that never arrived)
+ parser.readFrame();
+ Assert.assertTrue(output.getTrace(), output.getTrace().startsWith(
+ stream + "-HeadersStart\n" +
+ stream + "-Header-[:status]-[500]\n"));
+ output.clearTrace();
+
+ // reset frame
+ parser.readFrame();
+ Assert.assertEquals(stream + "-RST-[11]\n", output.getTrace());
+ output.clearTrace();
+
+ // Prepare buffers for re-use
+ headersPayload.clear();
+ dataFramePayload.clear();
+ }
+ }
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 8e39b4240c..02790bdbc7 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -121,6 +121,16 @@
</fix>
</changelog>
</subsection>
+ <subsection name="Coyote">
+ <changelog>
+ <fix>
+ When an HTTP/2 stream was reset, the current active stream count was not
+ reduced. If enough resets occurred on a connection, the current active
+ stream count limit was reached and no new streams could be created on
+ that connection. (markt)
+ </fix>
+ </changelog>
+ </subsection>
<subsection name="Other">
<changelog>
<scode>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org