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 14:02:22 UTC
[tomcat] branch 8.5.x updated (0e87d92 -> fbbbfc0)
This is an automated email from the ASF dual-hosted git repository.
markt pushed a change to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git.
from 0e87d92 Add setting direction to debug logging.
new 068cc31 Improve debug logging
new 7e344a6 Keep connection flow control window consistent with initial window size.
new fbbbfc0 Workaround https://bz.apache.org/bugzilla/show_bug.cgi?id=63690
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../apache/coyote/http2/Http2UpgradeHandler.java | 80 ++++++++++++++++++----
webapps/docs/changelog.xml | 13 ++++
webapps/docs/config/http2.xml | 29 ++++----
3 files changed, 95 insertions(+), 27 deletions(-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 02/03: Keep connection flow control window consistent with
initial window size.
Posted by ma...@apache.org.
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 7e344a6f84d9e30340ac532d011c244293e2f15f
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Sep 5 14:48:46 2019 +0100
Keep connection flow control window consistent with initial window size.
If the HTTP/2 connection requires an initial window size larger than the
default, send a WINDOW_UPDATE to increase the flow control window for
the connection so that the initial size of the flow control window for
the connection is consistent with the increased value.
---
.../apache/coyote/http2/Http2UpgradeHandler.java | 33 ++++++++++++++++++++++
webapps/docs/changelog.xml | 6 ++++
2 files changed, 39 insertions(+)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index d99e150..adeafa9 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -568,11 +568,21 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
}
+ /**
+ * Write the initial settings frame and any necessary supporting frames. If
+ * the initial settings increase the initial window size, it will also be
+ * necessary to send a WINDOW_UPDATE frame to increase the size of the flow
+ * control window for the connection (stream 0).
+ */
private void writeSettings() {
// Send the initial settings frame
try {
byte[] settings = localSettings.getSettingsFrameForPending();
socketWrapper.write(true, settings, 0, settings.length);
+ byte[] windowUpdateFrame = createWindowUpdateForSettings();
+ if (windowUpdateFrame.length > 0) {
+ socketWrapper.write(true, windowUpdateFrame, 0 , windowUpdateFrame.length);
+ }
socketWrapper.flush(true);
} catch (IOException ioe) {
String msg = sm.getString("upgradeHandler.sendPrefaceFail", connectionId);
@@ -584,6 +594,29 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
}
+ /**
+ * @return The WINDOW_UPDATE frame if one is required or an empty array if
+ * no WINDOW_UPDATE is required.
+ */
+ protected byte[] createWindowUpdateForSettings() {
+ // Build a WINDOW_UPDATE frame if one is required. If not, create an
+ // empty byte array.
+ byte[] windowUpdateFrame;
+ int increment = protocol.getInitialWindowSize() - ConnectionSettingsBase.DEFAULT_INITIAL_WINDOW_SIZE;
+ if (increment > 0) {
+ // Build window update frame for stream 0
+ windowUpdateFrame = new byte[13];
+ ByteUtil.setThreeBytes(windowUpdateFrame, 0, 4);
+ windowUpdateFrame[3] = FrameType.WINDOW_UPDATE.getIdByte();
+ ByteUtil.set31Bits(windowUpdateFrame, 9, increment);
+ } else {
+ windowUpdateFrame = new byte[0];
+ }
+
+ return windowUpdateFrame;
+ }
+
+
private void writeGoAwayFrame(int maxStreamId, long errorCode, byte[] debugMsg)
throws IOException {
byte[] fixedPayload = new byte[8];
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index d6d889b..16c6b8c 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -64,6 +64,12 @@
<code>overheadDataThreshold</code> and
<code>overheadWindowUpdateThreshold</code>. (markt)
</fix>
+ <fix>
+ If the HTTP/2 connection requires an initial window size larger than the
+ default, send a WINDOW_UPDATE to increase the flow control window for the
+ connection so that the initial size of the flow control window for the
+ connection is consistent with the increased value. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Web applications">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 01/03: Improve debug logging
Posted by ma...@apache.org.
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 068cc317c5fb4663514201c921b9c192fcb0e1d0
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Sep 5 13:28:45 2019 +0100
Improve debug logging
---
java/org/apache/coyote/http2/Http2UpgradeHandler.java | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 8867b58..d99e150 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -251,7 +251,7 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
} catch (Http2Exception e) {
String msg = sm.getString("upgradeHandler.invalidPreface", connectionId);
if (log.isDebugEnabled()) {
- log.debug(msg);
+ log.debug(msg, e);
}
throw new ProtocolException(msg);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 03/03: Workaround
https://bz.apache.org/bugzilla/show_bug.cgi?id=63690
Posted by ma...@apache.org.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org