This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 3a5556feff7e2be96e53c630f3c3e73a31adc975 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 9753ebc..10a1f65 100644 --- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java @@ -142,6 +142,8 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH // Track 'overhead' frames vs 'request/response' frames private final AtomicLong overheadCount = new AtomicLong(-10); + private volatile int lastNonFinalDataPayload; + private volatile int lastWindowUpdate; Http2UpgradeHandler(Http2Protocol protocol, Adapter adapter, Request coyoteRequest) { @@ -150,6 +152,9 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH 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); @@ -1372,15 +1377,21 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH // .. 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); } } @@ -1615,13 +1626,25 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH @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); @@ -1629,13 +1652,13 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH 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 b91c956..99504d0 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -61,6 +61,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 ebba1b7..02400e6 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