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:53 UTC
[tomcat] branch master updated (64c14b1 -> 3a5556f)
This is an automated email from the ASF dual-hosted git repository.
markt pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git.
from 64c14b1 Add setting direction to debug logging.
new 702705e Improve debug logging
new 69d7c04 Keep connection flow control window consistent with initial window size.
new 3a5556f 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:
.../coyote/http2/Http2AsyncUpgradeHandler.java | 4 +-
.../apache/coyote/http2/Http2UpgradeHandler.java | 80 ++++++++++++++++++----
webapps/docs/changelog.xml | 13 ++++
webapps/docs/config/http2.xml | 29 ++++----
4 files changed, 97 insertions(+), 29 deletions(-)
---------------------------------------------------------------------
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 master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 702705ea06172fbb3358770b98f7e4da7e294515
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 15385f6..f395d10 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -235,7 +235,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
} 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] 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 master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 69d7c0435c17444d5314da2d237e7468538e869a
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.
---
.../coyote/http2/Http2AsyncUpgradeHandler.java | 4 +--
.../apache/coyote/http2/Http2UpgradeHandler.java | 33 ++++++++++++++++++++++
webapps/docs/changelog.xml | 6 ++++
3 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
index 92ad29c..545292f 100644
--- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
@@ -105,10 +105,10 @@ public class Http2AsyncUpgradeHandler extends Http2UpgradeHandler {
@Override
protected void writeSettings() {
- // Send the initial settings frame
socketWrapper.write(BlockingMode.SEMI_BLOCK, protocol.getWriteTimeout(),
TimeUnit.MILLISECONDS, null, SocketWrapperBase.COMPLETE_WRITE, errorCompletion,
- ByteBuffer.wrap(localSettings.getSettingsFrameForPending()));
+ ByteBuffer.wrap(localSettings.getSettingsFrameForPending()),
+ ByteBuffer.wrap(createWindowUpdateForSettings()));
if (error != null) {
String msg = sm.getString("upgradeHandler.sendPrefaceFail", connectionId);
if (log.isDebugEnabled()) {
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index f395d10..9753ebc 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -560,11 +560,21 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
}
+ /**
+ * 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).
+ */
protected 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);
@@ -576,6 +586,29 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
}
+ /**
+ * @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;
+ }
+
+
protected 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 6e907c9..b91c956 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -68,6 +68,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] 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 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