This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 043d6581b86af0e553391965771f6715d79ca54c
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Sep 30 12:37:45 2024 +0100
Improve HTTP/2 handling of trailer headers
Prior to this change, trailers for a stream had to be received before
any headers of a subsequent stream. This commit removes that limitation.
The idle stream handling is removed since Tomcat never creates streams
in the idle state without immediately transitioning them to open.
---
.../apache/coyote/http2/Http2UpgradeHandler.java | 34 +++++++------------
.../apache/coyote/http2/TestHttp2Section_8_1.java | 38 +++++++++++++++++++---
2 files changed, 45 insertions(+), 27 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index d44982fc82..340621d931 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -118,8 +118,7 @@ class Http2UpgradeHandler extends AbstractStream implements
InternalHttpUpgradeH
private final ConcurrentNavigableMap<Integer,AbstractNonZeroStream>
streams = new ConcurrentSkipListMap<>();
protected final AtomicInteger activeRemoteStreamCount = new
AtomicInteger(0);
- // Start at -1 so the 'add 2' logic in closeIdleStreams() works
- private volatile int maxActiveRemoteStreamId = -1;
+ private volatile int maxActiveRemoteStreamId = 0;
private volatile int maxProcessedStreamId;
private final AtomicInteger nextLocalStreamId = new AtomicInteger(2);
private final PingManager pingManager = getPingManager();
@@ -178,7 +177,7 @@ class Http2UpgradeHandler extends AbstractStream implements
InternalHttpUpgradeH
Integer key = Integer.valueOf(1);
Stream stream = new Stream(key, this, coyoteRequest);
streams.put(key, stream);
- maxActiveRemoteStreamId = 1;
+ maxActiveRemoteStreamId = 0;
activeRemoteStreamCount.set(1);
maxProcessedStreamId = 1;
}
@@ -1597,16 +1596,19 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
if (connectionState.get().isNewStreamAllowed()) {
Stream stream = getStream(streamId, false);
if (stream == null) {
- stream = createRemoteStream(streamId);
- activeRemoteStreamCount.incrementAndGet();
- }
- if (streamId < maxActiveRemoteStreamId) {
- throw new
ConnectionException(sm.getString("upgradeHandler.stream.old",
Integer.valueOf(streamId),
- Integer.valueOf(maxActiveRemoteStreamId)),
Http2Error.PROTOCOL_ERROR);
+ // New stream
+ if (streamId > maxActiveRemoteStreamId) {
+ stream = createRemoteStream(streamId);
+ activeRemoteStreamCount.incrementAndGet();
+ maxActiveRemoteStreamId = streamId;
+ } else {
+ // ID for new stream must always be greater than any
previous stream
+ throw new
ConnectionException(sm.getString("upgradeHandler.stream.old",
Integer.valueOf(streamId),
+ Integer.valueOf(maxActiveRemoteStreamId)),
Http2Error.PROTOCOL_ERROR);
+ }
}
stream.checkState(FrameType.HEADERS);
stream.receivedStartOfHeaders(headersEndStream);
- closeIdleStreams(streamId);
return stream;
} else {
if (log.isTraceEnabled()) {
@@ -1619,18 +1621,6 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
}
- private void closeIdleStreams(int newMaxActiveRemoteStreamId) {
- final ConcurrentNavigableMap<Integer,AbstractNonZeroStream> subMap =
streams.subMap(
- Integer.valueOf(maxActiveRemoteStreamId), false,
Integer.valueOf(newMaxActiveRemoteStreamId), false);
- for (AbstractNonZeroStream stream : subMap.values()) {
- if (stream instanceof Stream) {
- ((Stream) stream).closeIfIdle();
- }
- }
- maxActiveRemoteStreamId = newMaxActiveRemoteStreamId;
- }
-
-
/**
* Unused - NO-OP.
*
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
b/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
index 278f266474..45a6864f09 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
@@ -64,19 +64,24 @@ public class TestHttp2Section_8_1 extends Http2TestBase {
ByteBuffer trailerPayload = ByteBuffer.allocate(256);
buildPostRequest(headersFrameHeader, headersPayload, false,
dataFrameHeader, dataPayload, null, true, 3);
- buildTrailerHeaders(trailerFrameHeader, trailerPayload, 3);
// Write the headers
writeFrame(headersFrameHeader, headersPayload);
// Body
writeFrame(dataFrameHeader, dataPayload);
+
+ sendSimpleGetRequest(5);
+
// Trailers
+ buildTrailerHeaders(trailerFrameHeader, trailerPayload, 3);
writeFrame(trailerFrameHeader, trailerPayload);
parser.readFrame();
parser.readFrame();
parser.readFrame();
parser.readFrame();
+ parser.readFrame();
+ parser.readFrame();
String len;
if (allowTrailerHeader) {
@@ -85,10 +90,33 @@ public class TestHttp2Section_8_1 extends Http2TestBase {
len = "256";
}
- Assert.assertEquals("0-WindowSize-[256]\n" + "3-WindowSize-[256]\n" +
"3-HeadersStart\n" +
- "3-Header-[:status]-[200]\n" + "3-Header-[content-length]-[" +
len + "]\n" + "3-Header-[date]-[" +
- DEFAULT_DATE + "]\n" + "3-HeadersEnd\n" + "3-Body-" + len +
"\n" + "3-EndOfStream\n",
- output.getTrace());
+ /*
+ * There is no guaranteed order between stream 3 and stream 5. It all
depends on server side timing. Also need
+ * to ensure no unexpected frames are received.
+ */
+ String stream0WindowSize = "0-WindowSize-[256]\n";
+ String stream3WindowSize = "3-WindowSize-[256]\n";
+ String stream3Headers = "3-HeadersStart\n" +
"3-Header-[:status]-[200]\n" +
+ "3-Header-[content-length]-[" + len + "]\n" +
"3-Header-[date]-[" + DEFAULT_DATE + "]\n" +
+ "3-HeadersEnd\n";
+ String stream3Body = "3-Body-" + len + "\n" + "3-EndOfStream\n";
+ String stream5Headers = "5-HeadersStart\n" +
"5-Header-[:status]-[200]\n" +
+ "5-Header-[content-type]-[application/octet-stream]\n" +
"5-Header-[content-length]-[8192]\n" +
+ "5-Header-[date]-[" + DEFAULT_DATE + "]\n" + "5-HeadersEnd\n";
+ String stream5Body = "5-Body-8192\n" + "5-EndOfStream\n";
+
+ String trace = output.getTrace();
+ System.out.println(trace);
+
+ Assert.assertTrue(trace, trace.contains(stream0WindowSize));
+ Assert.assertTrue(trace, trace.contains(stream3WindowSize));
+ Assert.assertTrue(trace, trace.contains(stream3Headers));
+ Assert.assertTrue(trace, trace.contains(stream3Body));
+ Assert.assertTrue(trace, trace.contains(stream5Headers));
+ Assert.assertTrue(trace, trace.contains(stream5Body));
+ Assert.assertEquals("Unexpected data in trace - " + trace,
stream0WindowSize.length() +
+ stream3WindowSize.length() + stream3Headers.length() +
stream3Body.length() + stream5Headers.length() +
+ stream5Body.length(), trace.length());
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
