This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 10.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.0.x by this push:
new 6e5e61c Fix problems with file uploads and HTTP/2
6e5e61c is described below
commit 6e5e61c33e0b1bb108c26f4cde866668058667d4
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Jun 14 21:15:30 2021 +0100
Fix problems with file uploads and HTTP/2
Small writes from the user agent were triggering small window updates
which in turn triggered more small writes. With lots of small writes the
overhead protection code was activated and the connection closed.
---
.../coyote/http2/Http2AsyncUpgradeHandler.java | 30 ++++++++++++-------
.../apache/coyote/http2/Http2UpgradeHandler.java | 34 +++++++++++++++-------
.../apache/coyote/http2/LocalStrings.properties | 2 ++
java/org/apache/coyote/http2/Stream.java | 24 +++++++++++++++
webapps/docs/changelog.xml | 10 +++++++
5 files changed, 79 insertions(+), 21 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
index b77fee6..6d7e3eb 100644
--- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
@@ -231,22 +231,32 @@ public class Http2AsyncUpgradeHandler extends
Http2UpgradeHandler {
@Override
void writeWindowUpdate(AbstractNonZeroStream stream, int increment,
boolean applicationInitiated)
throws IOException {
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("upgradeHandler.windowUpdateConnection",
+ getConnectionId(), Integer.valueOf(increment)));
+ }
// Build window update frame for stream 0
byte[] frame = new byte[13];
ByteUtil.setThreeBytes(frame, 0, 4);
frame[3] = FrameType.WINDOW_UPDATE.getIdByte();
ByteUtil.set31Bits(frame, 9, increment);
// No need to send update from closed stream
- if (stream instanceof Stream && ((Stream) stream).canWrite()) {
- // Change stream Id
- byte[] frame2 = new byte[13];
- ByteUtil.setThreeBytes(frame2, 0, 4);
- frame2[3] = FrameType.WINDOW_UPDATE.getIdByte();
- ByteUtil.set31Bits(frame2, 9, increment);
- ByteUtil.set31Bits(frame2, 5, stream.getIdAsInt());
- socketWrapper.write(BlockingMode.SEMI_BLOCK,
protocol.getWriteTimeout(),
- TimeUnit.MILLISECONDS, null,
SocketWrapperBase.COMPLETE_WRITE, errorCompletion,
- ByteBuffer.wrap(frame), ByteBuffer.wrap(frame2));
+ if (stream instanceof Stream && ((Stream) stream).canWrite()) {
+ int streamIncrement = ((Stream)
stream).getWindowUpdateSizeToWrite(increment);
+ if (streamIncrement > 0) {
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("upgradeHandler.windowUpdateStream",
+ getConnectionId(), getIdAsString(),
Integer.valueOf(streamIncrement)));
+ }
+ byte[] frame2 = new byte[13];
+ ByteUtil.setThreeBytes(frame2, 0, 4);
+ frame2[3] = FrameType.WINDOW_UPDATE.getIdByte();
+ ByteUtil.set31Bits(frame2, 9, streamIncrement);
+ ByteUtil.set31Bits(frame2, 5, stream.getIdAsInt());
+ socketWrapper.write(BlockingMode.SEMI_BLOCK,
protocol.getWriteTimeout(),
+ TimeUnit.MILLISECONDS, null,
SocketWrapperBase.COMPLETE_WRITE, errorCompletion,
+ ByteBuffer.wrap(frame), ByteBuffer.wrap(frame2));
+ }
} else {
socketWrapper.write(BlockingMode.SEMI_BLOCK,
protocol.getWriteTimeout(),
TimeUnit.MILLISECONDS, null,
SocketWrapperBase.COMPLETE_WRITE, errorCompletion,
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 994a7cb..fa650b7 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -804,6 +804,10 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
*/
void writeWindowUpdate(AbstractNonZeroStream stream, int increment,
boolean applicationInitiated)
throws IOException {
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("upgradeHandler.windowUpdateConnection",
+ getConnectionId(), Integer.valueOf(increment)));
+ }
synchronized (socketWrapper) {
// Build window update frame for stream 0
byte[] frame = new byte[13];
@@ -812,17 +816,25 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
ByteUtil.set31Bits(frame, 9, increment);
socketWrapper.write(true, frame, 0, frame.length);
// No need to send update from closed stream
- if (stream instanceof Stream && ((Stream) stream).canWrite()) {
- // Change stream Id and re-use
- ByteUtil.set31Bits(frame, 5, stream.getIdAsInt());
- try {
- socketWrapper.write(true, frame, 0, frame.length);
- socketWrapper.flush(true);
- } catch (IOException ioe) {
- if (applicationInitiated) {
- handleAppInitiatedIOException(ioe);
- } else {
- throw ioe;
+ if (stream instanceof Stream && ((Stream) stream).canWrite()) {
+ int streamIncrement = ((Stream)
stream).getWindowUpdateSizeToWrite(increment);
+ if (streamIncrement > 0) {
+ if (log.isDebugEnabled()) {
+
log.debug(sm.getString("upgradeHandler.windowUpdateStream",
+ getConnectionId(), getIdAsString(),
Integer.valueOf(streamIncrement)));
+ }
+ // Re-use buffer as connection update has already been
written
+ ByteUtil.set31Bits(frame, 5, stream.getIdAsInt());
+ ByteUtil.set31Bits(frame, 9, streamIncrement);
+ try {
+ socketWrapper.write(true, frame, 0, frame.length);
+ socketWrapper.flush(true);
+ } catch (IOException ioe) {
+ if (applicationInitiated) {
+ handleAppInitiatedIOException(ioe);
+ } else {
+ throw ioe;
+ }
}
}
} else {
diff --git a/java/org/apache/coyote/http2/LocalStrings.properties
b/java/org/apache/coyote/http2/LocalStrings.properties
index 3e114d8..25858cb 100644
--- a/java/org/apache/coyote/http2/LocalStrings.properties
+++ b/java/org/apache/coyote/http2/LocalStrings.properties
@@ -158,6 +158,8 @@ upgradeHandler.upgradeDispatch.entry=Entry, Connection
[{0}], SocketStatus [{1}]
upgradeHandler.upgradeDispatch.exit=Exit, Connection [{0}], SocketState [{1}]
upgradeHandler.windowSizeReservationInterrupted=Connection [{0}], Stream
[{1}], reservation for [{2}] bytes
upgradeHandler.windowSizeTooBig=Connection [{0}], Stream [{1}], Window size
too big
+upgradeHandler.windowUpdateConnection=Connection [{0}], Sent window update to
client increasing window by [{1}] bytes
+upgradeHandler.windowUpdateStream=Connection [{0}], Stream [{1}], Sent window
update to client increasing window by [{2}] bytes
upgradeHandler.writeBody=Connection [{0}], Stream [{1}], Data length [{2}],
EndOfStream [{3}]
upgradeHandler.writeHeaders=Connection [{0}], Stream [{1}], Writing the
headers, EndOfStream [{2}]
upgradeHandler.writePushHeaders=Connection [{0}], Stream [{1}], Pushed stream
[{2}], EndOfStream [{3}]
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index 28ec14e..8d93d98 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -82,6 +82,9 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
private volatile StringBuilder cookieHeader = null;
+ private Object pendingWindowUpdateForStreamLock = new Object();
+ private int pendingWindowUpdateForStream = 0;
+
Stream(Integer identifier, Http2UpgradeHandler handler) {
this(identifier, handler, null);
@@ -744,6 +747,27 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
+ int getWindowUpdateSizeToWrite(int increment) {
+ int result;
+ int threshold =
handler.getProtocol().getOverheadWindowUpdateThreshold();
+ synchronized (pendingWindowUpdateForStreamLock) {
+ if (increment > threshold) {
+ result = increment + pendingWindowUpdateForStream;
+ pendingWindowUpdateForStream = 0;
+ } else {
+ pendingWindowUpdateForStream += increment;
+ if (pendingWindowUpdateForStream > threshold) {
+ result = pendingWindowUpdateForStream;
+ pendingWindowUpdateForStream = 0;
+ } else {
+ result = 0;
+ }
+ }
+ }
+ return result;
+ }
+
+
private static void push(final Http2UpgradeHandler handler, final Request
request,
final Stream stream) throws IOException {
if (org.apache.coyote.Constants.IS_SECURITY_ENABLED) {
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index c32169e..464fc7b 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -112,6 +112,16 @@
connections. Treat them the same way as clean closes of non-TLS
connections rather than as unknown errors. (markt)
</fix>
+ <fix>
+ Modify the HTTP/2 connector not to sent small updates for stream flow
+ control windows to the user agent as, depending on how the user agent
is
+ written, this may trigger small writes from the user agent that in turn
+ trigger the overhead protection. Small updates for stream flow control
+ windows are now combined with subsequent flow control window updates
for
+ that stream to ensure that all stream flow control window updates sent
+ from Tomcat are larger than <code>overheadWindowUpdateThreshold</code>.
+ (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Other">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
