From 9af2cb4bd21499a4c1779c7cc6bab146e7c1cf0b Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Thu, 25 Jan 2024 06:13:55 -0600 Subject: [PATCH 1/4] Report HTTP/2 header parsing errors earlier --- java/org/apache/coyote/http2/Http2Parser.java | 11 ++++++----- webapps/docs/changelog.xml | 5 +++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/java/org/apache/coyote/http2/Http2Parser.java b/java/org/apache/coyote/http2/Http2Parser.java index 72c30532e125..8588d15bb6ff 100644 --- a/java/org/apache/coyote/http2/Http2Parser.java +++ b/java/org/apache/coyote/http2/Http2Parser.java @@ -289,6 +289,9 @@ protected void readHeadersFrame(int streamId, int flags, int payloadSize, ByteBu swallowPayload(streamId, FrameType.HEADERS.getId(), padLength, true, buffer); + // Validate the headers so far + hpackDecoder.getHeaderEmitter().validateHeaders(); + if (Flags.isEndOfHeaders(flags)) { onHeadersComplete(streamId); } else { @@ -467,6 +470,9 @@ protected void readContinuationFrame(int streamId, int flags, int payloadSize, B readHeaderPayload(streamId, payloadSize, buffer); + // Validate the headers so far + hpackDecoder.getHeaderEmitter().validateHeaders(); + if (endOfHeaders) { headersCurrentStream = -1; onHeadersComplete(streamId); @@ -633,11 +639,6 @@ protected void onHeadersComplete(int streamId) throws Http2Exception { Http2Error.COMPRESSION_ERROR); } - // Delay validation (and triggering any exception) until this point - // since all the headers still have to be read if a StreamException is - // going to be thrown. - hpackDecoder.getHeaderEmitter().validateHeaders(); - synchronized (output) { output.headersEnd(streamId); diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 84b8d672c4bd..d22338977939 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -267,6 +267,11 @@ Fix a regression in refactoring for Hashtables which caused mbeans to lose many of their attributes. (remm) + + Improve error reporting to HTTP/2 clients for header processing errors + by reporting problems at the end of the frame where the error was + detected rather than at the end of the headers. (markt) + From a312e41705d01a3319995f951931d0ce66466869 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Thu, 25 Jan 2024 06:20:14 -0600 Subject: [PATCH 2/4] Make recycled streams eligible for GC immediately. Improves scalability. --- java/org/apache/coyote/http2/Http2UpgradeHandler.java | 2 +- java/org/apache/coyote/http2/Stream.java | 2 +- webapps/docs/changelog.xml | 5 +++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java index 3285a4674368..bd4a1f845040 100644 --- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java @@ -92,7 +92,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH private static final String HTTP2_SETTINGS_HEADER = "HTTP2-Settings"; - private static final HeaderSink HEADER_SINK = new HeaderSink(); + protected static final HeaderSink HEADER_SINK = new HeaderSink(); private final Object priorityTreeLock = new Object(); diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java index 304187771e1d..967643ff6412 100644 --- a/java/org/apache/coyote/http2/Stream.java +++ b/java/org/apache/coyote/http2/Stream.java @@ -492,7 +492,7 @@ public void validateHeaders() throws StreamException { if (headerException == null) { return; } - + handler.getHpackDecoder().setHeaderEmitter(Http2UpgradeHandler.HEADER_SINK); throw headerException; } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index d22338977939..3a75374417a8 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -189,6 +189,11 @@ Improvements to HTTP/2 overhead protection. (markt) + + Remove the remaining reference to a stream once the stream has been + recycled. This makes the stream eligible for garbage collection earlier + and thereby improves scalability. (markt) + From 9e2dcbf8b1d1a3fa84272a4059758b0adaf0b9fd Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Thu, 25 Jan 2024 08:48:30 -0600 Subject: [PATCH 3/4] Update tests after HTTP/2 improvements --- test/org/apache/coyote/http2/TestHttp2Limits.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test/org/apache/coyote/http2/TestHttp2Limits.java b/test/org/apache/coyote/http2/TestHttp2Limits.java index 3b07bad43e1d..01243a8c1899 100644 --- a/test/org/apache/coyote/http2/TestHttp2Limits.java +++ b/test/org/apache/coyote/http2/TestHttp2Limits.java @@ -168,7 +168,7 @@ public void testHeaderLimits1x32kin1kChunks() throws Exception { // 500ms per frame write delay to give server a chance to process the // stream reset and the connection reset before the request is fully // sent. - doTestHeaderLimits(1, 32*1024, 1024, 500, FailureMode.CONNECTION_RESET); + doTestHeaderLimits(1, 32 * 1024, 1024, 500, FailureMode.STREAM_RESET_THEN_CONNECTION_RESET); } @@ -286,6 +286,13 @@ private void doTestHeaderLimits(int headerCount, int headerSize, int maxHeaderPa Assert.assertNull(e); break; } + case STREAM_RESET_THEN_CONNECTION_RESET: { + // Expect a stream reset + parser.readFrame(); + Assert.assertEquals("3-RST-[11]\n", output.getTrace()); + output.clearTrace(); + } + //$FALL-THROUGH$ case CONNECTION_RESET: { // This message uses i18n and needs to be used in a regular // expression (since we don't know the connection ID). Generate the @@ -541,6 +548,10 @@ private void doTestPostWithTrailerHeaders(int maxTrailerCount, int maxTrailerSiz Assert.assertEquals("3-RST-[11]\n", output.getTrace()); break; } + case STREAM_RESET_THEN_CONNECTION_RESET: { + Assert.fail("Not used"); + break; + } case CONNECTION_RESET: { // NIO2 can sometimes send window updates depending timing skipWindowSizeFrames(); @@ -563,7 +574,7 @@ private enum FailureMode { NONE, STREAM_RESET, CONNECTION_RESET, - + STREAM_RESET_THEN_CONNECTION_RESET, } From d380718a383d948f33b417f96da2efb3b259de53 Mon Sep 17 00:00:00 2001 From: Cesar Hernandez Date: Wed, 3 Apr 2024 10:03:00 -0600 Subject: [PATCH 4/4] Prepare for release 10.0.28-TT.8 --- build.properties.default | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.properties.default b/build.properties.default index 107480d69c97..3329ecae1d7d 100644 --- a/build.properties.default +++ b/build.properties.default @@ -33,7 +33,7 @@ version.major=10 version.minor=0 version.build=28 version.patch=0 -version.suffix=-TT.7 +version.suffix=-TT.8 version.dev= # ----- Build tools -----