This is an automated email from the ASF dual-hosted git repository.
markt-asf 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 8effbb4a50 Review "Fix flow control over-allocation when releasing the
HTTP/2..."
8effbb4a50 is described below
commit 8effbb4a50846567fa228d74dc856c3345587e08
Author: Mark Thomas <[email protected]>
AuthorDate: Mon Nov 2 16:14:46 2026 +0000
Review "Fix flow control over-allocation when releasing the HTTP/2..."
Fix allocation leak
Co-authored by: Claude / Sonnet 5.5
---
.../apache/coyote/http2/Http2UpgradeHandler.java | 24 ++++
test/org/apache/coyote/http2/TestRfc9218.java | 134 ++++++++++++++-------
webapps/docs/changelog.xml | 4 +-
3 files changed, 117 insertions(+), 45 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 1345ab32e9..c77cf607c4 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1425,6 +1425,30 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
}
}
}
+
+ /*
+ * Rounding down the proportional shares may leave part of the
increment unallocated. The allocations
+ * on this path are the only record of the connection window
the client made available so allocate
+ * what is left, in priority order, to the streams that are
still waiting. The streams in the back log
+ * are still waiting for at least the remaining increment (the
back log is larger than the increment)
+ * so all of it can be allocated. Otherwise the client could
be waiting for data the server will not
+ * send until it receives a further window update the client
has no reason to send.
+ */
+ if (remaining > 0) {
+ for (Stream s : orderedStreams) {
+ if (s.getConnectionAllocationRequested() < 1) {
+ continue;
+ }
+ remaining = allocate(s, remaining);
+ result.add(s);
+ if (s.getConnectionAllocationRequested() == 0) {
+ backLogStreams.remove(s);
+ }
+ if (remaining < 1) {
+ break;
+ }
+ }
+ }
}
return result;
} finally {
diff --git a/test/org/apache/coyote/http2/TestRfc9218.java
b/test/org/apache/coyote/http2/TestRfc9218.java
index 9e434dbd81..cd23a5df2e 100644
--- a/test/org/apache/coyote/http2/TestRfc9218.java
+++ b/test/org/apache/coyote/http2/TestRfc9218.java
@@ -25,52 +25,43 @@ import org.junit.Test;
public class TestRfc9218 extends Http2TestBase {
@Test
- public void testPriority() throws Exception {
- http2Connect();
+ public void testBackLogRoundingRemainderAllocated() throws Exception {
+ blockStreams();
+
+ // Make all three streams incremental with the same urgency so the
increment is shared proportionally
+ sendPriorityUpdate(17, 3, true);
+ sendPriorityUpdate(19, 3, true);
+ sendPriorityUpdate(21, 3, true);
/*
- * This test uses small window updates and data frames that will
trigger the excessive overhead protection so
- * disable it.
+ * Add 10 bytes to the connection window. The proportional shares are
3.04, 3.47 and 3.47 bytes. Rounding each
+ * down allocates 9 bytes. The remaining byte is part of the window
the client made available so it must be
+ * allocated rather than lost. It goes to 17 since that is first in
priority order.
*/
- http2Protocol.setOverheadWindowUpdateThreshold(0);
- http2Protocol.setOverheadDataThreshold(0);
-
- // Default connection window size is 64k - 1. Initial request will
have used 8k (56k -1). Increase it to 57k.
- sendWindowUpdate(0, 1 + 1024);
-
- // Consume 56k of the connection window
- for (int i = 3; i < 17; i += 2) {
- sendSimpleGetRequest(i);
- readSimpleGetResponse();
- }
+ sendWindowUpdate(0, 10);
+ parser.readFrame();
+ parser.readFrame();
+ parser.readFrame();
String trace = output.getTrace();
System.out.println(trace);
+ Assert.assertTrue(trace, trace.contains("17-Body-4\n"));
+ trace = trace.replace("17-Body-4\n", "");
+ Assert.assertTrue(trace, trace.contains("19-Body-3\n"));
+ trace = trace.replace("19-Body-3\n", "");
+ Assert.assertTrue(trace, trace.contains("21-Body-3\n"));
+ trace = trace.replace("21-Body-3\n", "");
+ Assert.assertEquals(0, trace.length());
output.clearTrace();
+ }
- // At this point the connection window should be 1k
-
- // Process a request on stream 17. This should consume the connection
window.
- sendSimpleGetRequest(17);
- // 17-headers, 17-1k-body
- parser.readFrame();
- parser.readFrame();
- trace = output.getTrace();
- System.out.println(trace);
- output.clearTrace();
- // Send additional requests. Connection window is empty so only
headers will be returned.
- sendSimpleGetRequest(19);
- sendSimpleGetRequest(21);
+ @Test
+ public void testPriority() throws Exception {
+ blockStreams();
- // 19-headers, 21-headers
- parser.readFrame();
- parser.readFrame();
- trace = output.getTrace();
- System.out.println(trace);
- output.clearTrace();
+ String trace;
- // At this point 17, 19 and 21 are all blocked because the connection
window is zero.
// 17 - 7k body left
// 19 - 8k body left
// 21 - 8k body left
@@ -117,8 +108,8 @@ public class TestRfc9218 extends Http2TestBase {
trace = output.getTrace();
System.out.println(trace);
- Assert.assertTrue(trace.contains("17-Body-877\n"));
- trace = trace.replace("17-Body-877\n", "");
+ Assert.assertTrue(trace.contains("17-Body-878\n"));
+ trace = trace.replace("17-Body-878\n", "");
Assert.assertTrue(trace.contains("19-Body-1170\n"));
trace = trace.replace("19-Body-1170\n", "");
Assert.assertTrue(trace.contains("21-Body-1024\n"));
@@ -126,7 +117,7 @@ public class TestRfc9218 extends Http2TestBase {
Assert.assertEquals(0, trace.length());
output.clearTrace();
- // 17 - 5267 body left
+ // 17 - 5266 body left
// 19 - 7022 body left
// 21 - 6144 body left
@@ -141,7 +132,7 @@ public class TestRfc9218 extends Http2TestBase {
Assert.assertEquals("17-Body-1\n", trace);
output.clearTrace();
- // 17 - 5266 body left
+ // 17 - 5265 body left
// 19 - 7022 body left
// 21 - 6144 body left
@@ -169,17 +160,17 @@ public class TestRfc9218 extends Http2TestBase {
trace = output.getTrace();
System.out.println(trace);
- Assert.assertTrue(trace.contains("17-Body-5266\n"));
- trace = trace.replace("17-Body-5266\n", "");
+ Assert.assertTrue(trace.contains("17-Body-5265\n"));
+ trace = trace.replace("17-Body-5265\n", "");
Assert.assertTrue(trace.contains("17-EndOfStream\n"));
trace = trace.replace("17-EndOfStream\n", "");
- Assert.assertTrue(trace.contains("19-Body-1560\n"));
- trace = trace.replace("19-Body-1560\n", "");
+ Assert.assertTrue(trace.contains("19-Body-1562\n"));
+ trace = trace.replace("19-Body-1562\n", "");
Assert.assertTrue(trace.contains("21-Body-1365\n"));
trace = trace.replace("21-Body-1365\n", "");
Assert.assertEquals(0, trace.length());
- // 19 - 5462 body left
+ // 19 - 5460 body left
// 21 - 4779 body left
// Add 16k to the connection window. Should fully allocate 19 and 21.
@@ -195,6 +186,61 @@ public class TestRfc9218 extends Http2TestBase {
}
+ /*
+ * Opens the connection and leaves streams 17, 19 and 21 blocked because
the connection window is zero.
+ *
+ * 17 - 7k body left
+ * 19 - 8k body left
+ * 21 - 8k body left
+ */
+ private void blockStreams() throws Exception {
+ http2Connect();
+
+ /*
+ * This test uses small window updates and data frames that will
trigger the excessive overhead protection so
+ * disable it.
+ */
+ http2Protocol.setOverheadWindowUpdateThreshold(0);
+ http2Protocol.setOverheadDataThreshold(0);
+
+ // Default connection window size is 64k - 1. Initial request will
have used 8k (56k -1). Increase it to 57k.
+ sendWindowUpdate(0, 1 + 1024);
+
+ // Consume 56k of the connection window
+ for (int i = 3; i < 17; i += 2) {
+ sendSimpleGetRequest(i);
+ readSimpleGetResponse();
+ }
+
+ String trace = output.getTrace();
+ System.out.println(trace);
+ output.clearTrace();
+
+ // At this point the connection window should be 1k
+
+ // Process a request on stream 17. This should consume the connection
window.
+ sendSimpleGetRequest(17);
+ // 17-headers, 17-1k-body
+ parser.readFrame();
+ parser.readFrame();
+ trace = output.getTrace();
+ System.out.println(trace);
+ output.clearTrace();
+
+ // Send additional requests. Connection window is empty so only
headers will be returned.
+ sendSimpleGetRequest(19);
+ sendSimpleGetRequest(21);
+
+ // 19-headers, 21-headers
+ parser.readFrame();
+ parser.readFrame();
+ trace = output.getTrace();
+ System.out.println(trace);
+ output.clearTrace();
+
+ // At this point 17, 19 and 21 are all blocked because the connection
window is zero.
+ }
+
private void sendInvalidPriorityUpdate(int streamId) throws IOException {
byte[] payload = "u=1:i".getBytes(StandardCharsets.US_ASCII);
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 6f9fb937d6..6b7ed6ef0f 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -351,7 +351,9 @@
compliant client must treat as a connection error of type
<code>FLOW_CONTROL_ERROR</code>. Streams that are left without an
allocation remain in the back log and are considered again when the
- next window increment arrives. (remm)
+ next window increment arrives. Any part of the increment left
+ unallocated by the rounding of the proportional shares is allocated to
+ the waiting streams in priority order rather than being lost. (remm)
</fix>
<fix>
Permit the <code>WINDOW_UPDATE</code> and <code>RST_STREAM</code>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
