This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 8.5.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit fbbbfc03c8b9b26dff68c19fe9d8f5f95303928a Author: Mark Thomas <ma...@apache.org> AuthorDate: Thu Sep 5 12:27:24 2019 +0100 Workaround https://bz.apache.org/bugzilla/show_bug.cgi?id=63690 Use the average of the current and previous sizes when calculating overhead for HTTP/2 DATA and WINDOW_UPDATE frames to avoid false positives as a result of client side buffering behaviour that causes a small percentage of non-final DATA frames to be smaller than expected. --- .../apache/coyote/http2/Http2UpgradeHandler.java | 45 ++++++++++++++++------ webapps/docs/changelog.xml | 7 ++++ webapps/docs/config/http2.xml | 29 +++++++------- 3 files changed, 55 insertions(+), 26 deletions(-) diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java index adeafa9..0b7c05a 100644 --- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java @@ -156,6 +156,8 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU // Track 'overhead' frames vs 'request/response' frames private final AtomicLong overheadCount = new AtomicLong(-10); + private volatile int lastNonFinalDataPayload; + private volatile int lastWindowUpdate; /** @@ -176,6 +178,9 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU this.adapter = adapter; this.connectionId = Integer.toString(connectionIdGenerator.getAndIncrement()); + lastNonFinalDataPayload = protocol.getOverheadDataThreshold() * 2; + lastWindowUpdate = protocol.getOverheadWindowUpdateThreshold() * 2; + remoteSettings = new ConnectionSettingsRemote(connectionId); localSettings = new ConnectionSettingsLocal(connectionId); @@ -1492,15 +1497,21 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU // .. but lots of small payloads are inefficient so that will increase // the overhead count unless it is the final DATA frame where small // payloads are expected. + + // See also https://bz.apache.org/bugzilla/show_bug.cgi?id=63690 + // The buffering behaviour of some clients means that small data frames + // are much more frequent (roughly 1 in 20) than expected. Use an + // average over two frames to avoid false positives. if (!endOfStream) { int overheadThreshold = protocol.getOverheadDataThreshold(); - if (payloadSize < overheadThreshold) { - if (payloadSize == 0) { - // Avoid division by zero - overheadCount.addAndGet(overheadThreshold); - } else { - overheadCount.addAndGet(overheadThreshold / payloadSize); - } + int average = (lastNonFinalDataPayload >> 1) + (payloadSize >> 1); + lastNonFinalDataPayload = payloadSize; + // Avoid division by zero + if (average == 0) { + average = 1; + } + if (average < overheadThreshold) { + overheadCount.addAndGet(overheadThreshold / average); } } @@ -1735,13 +1746,25 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU @Override public void incrementWindowSize(int streamId, int increment) throws Http2Exception { + // See also https://bz.apache.org/bugzilla/show_bug.cgi?id=63690 + // The buffering behaviour of some clients means that small data frames + // are much more frequent (roughly 1 in 20) than expected. Some clients + // issue a Window update for every DATA frame so a similar pattern may + // be observed. Use an average over two frames to avoid false positives. + + int average = (lastWindowUpdate >> 1) + (increment >> 1); int overheadThreshold = protocol.getOverheadWindowUpdateThreshold(); + lastWindowUpdate = increment; + // Avoid division by zero + if (average == 0) { + average = 1; + } if (streamId == 0) { // Check for small increments which are inefficient - if (increment < overheadThreshold) { + if (average < overheadThreshold) { // The smaller the increment, the larger the overhead - overheadCount.addAndGet(overheadThreshold / increment); + overheadCount.addAndGet(overheadThreshold / average); } incrementWindowSize(increment); @@ -1749,13 +1772,13 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU Stream stream = getStream(streamId, true); // Check for small increments which are inefficient - if (increment < overheadThreshold) { + if (average < overheadThreshold) { // For Streams, client might only release the minimum so check // against current demand BacklogTracker tracker = backLogStreams.get(stream); if (tracker == null || increment < tracker.getRemainingReservation()) { // The smaller the increment, the larger the overhead - overheadCount.addAndGet(overheadThreshold / increment); + overheadCount.addAndGet(overheadThreshold / average); } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 16c6b8c..79cc306 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -57,6 +57,13 @@ <subsection name="Coyote"> <changelog> <fix> + <bug>63690</bug>: Use the average of the current and previous sizes when + calculating overhead for HTTP/2 <code>DATA</code> and + <code>WINDOW_UPDATE</code> frames to avoid false positives as a result + of client side buffering behaviour that causes a small percentage of + non-final DATA frames to be smaller than expected. (markt) + </fix> + <fix> <bug>63706</bug>: Avoid NPE accessing https port with plaintext. (remm) </fix> <fix> diff --git a/webapps/docs/config/http2.xml b/webapps/docs/config/http2.xml index 9efafbd..1b2ad1f 100644 --- a/webapps/docs/config/http2.xml +++ b/webapps/docs/config/http2.xml @@ -214,28 +214,27 @@ </attribute> <attribute name="overheadDataThreshold" required="false"> - <p>The threshold below which the payload size of a non-final - <code>DATA</code> frame will trigger an increase in the overhead count - (see <strong>overheadCountFactor</strong>). The overhead count will be - increased by <code>overheadDataThreshold/payloadSize</code> so that the - smaller the <code>DATA</code> frame, the greater the increase in the - overhead count. A value of zero or less disables the checking of non-final - <code>DATA</code> frames. If not specified, a default value of - <code>1024</code> will be used.</p> + <p>The threshold below which the average payload size of the current and + previous non-final <code>DATA</code> frames will trigger an increase in + the overhead count (see <strong>overheadCountFactor</strong>). The + overhead count will be increased by + <code>overheadDataThreshold/average</code> so that the smaller the + average, the greater the increase in the overhead count. A value of zero + or less disables the checking of non-final <code>DATA</code> frames. If + not specified, a default value of <code>1024</code> will be used.</p> </attribute> <attribute name="overheadWindowUpdateThreshold" required="false"> - <p>The threshold below which the size of a <code>WINDOW_UPDATE</code> - frame will trigger an increase in the overhead count (see - <strong>overheadCountFactor</strong>). The overhead count will be - increased by <code>overheadWindowUpdateThreshold/updateSize</code> so - that the smaller the <code>WINDOW_UPDATE</code>, the greater the increase - in the overhead count. A value of zero or less disables the checking of + <p>The threshold below which the average size of current and previous + <code>WINDOW_UPDATE</code> frame will trigger an increase in the overhead + count (see <strong>overheadCountFactor</strong>). The overhead count will + be increased by <code>overheadWindowUpdateThreshold/average</code> so + that the smaller the average, the greater the increase in the overhead + count. A value of zero or less disables the checking of <code>WINDOW_UPDATE</code> frames. If not specified, a default value of <code>1024</code> will be used.</p> </attribute> - <attribute name="readTimeout" required="false"> <p>The time, in milliseconds, that Tomcat will wait for additional data when a partial HTTP/2 frame has been received. Negative values will be --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org