You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/09/05 13:59:56 UTC
[tomcat] 03/03: Workaround
https://bz.apache.org/bugzilla/show_bug.cgi?id=63690
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org