This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 2362172583 Fix bug with HTTP/2 stream reset and active connection count
2362172583 is described below
commit 2362172583519ca388c9980fda82f8e2c18ec6fb
Author: Mark Thomas <[email protected]>
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 | 6 +++
5 files changed, 81 insertions(+), 6 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
index 9f4af14e24..ce230b7de5 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 8ead0b378d..8d402c64b9 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -599,7 +599,11 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
socketWrapper.getLock().lock();
try {
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 ba11706257..301450011c 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -149,6 +149,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)
@@ -162,7 +167,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" +
@@ -626,16 +631,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);
http2Protocol.setHttp11Protocol((AbstractHttp11Protocol<?>)
connector.getProtocolHandler());
connector.addUpgradeProtocol(http2Protocol);
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 64b1d76069..7f4ec93b79 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -190,6 +190,12 @@
valid for the configured character set and throw an exception if not.
(markt)
</fix>
+ <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="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
