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 2020/08/18 20:14:22 UTC
[tomcat] branch 8.5.x updated (50ae519 -> d56541f)
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 50ae519 Re-word
new 32c2e19 Refactor for more consistent error code after client sends stream reset
new f126ebf Improve handling of flow control errors.
new d56541f Expand to ensure clean closure with connection flow control error
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 | 7 +--
java/org/apache/coyote/http2/Stream.java | 1 +
.../coyote/http2/WindowAllocationManager.java | 38 ++++++------
.../apache/coyote/http2/TestHttp2Section_5_1.java | 69 ++++++++++++++++++++--
webapps/docs/changelog.xml | 10 ++++
5 files changed, 99 insertions(+), 26 deletions(-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 02/03: Improve handling of flow control errors.
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 f126ebf390dbc4ad7ccc1e2b183688471e6282b0
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Aug 18 20:50:01 2020 +0100
Improve handling of flow control errors.
---
java/org/apache/coyote/http2/Stream.java | 1 +
.../coyote/http2/WindowAllocationManager.java | 38 ++++++++++++----------
.../apache/coyote/http2/TestHttp2Section_5_1.java | 4 +--
webapps/docs/changelog.xml | 5 +++
4 files changed, 29 insertions(+), 19 deletions(-)
diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
index 1927fe7..3a52074 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -677,6 +677,7 @@ public class Stream extends AbstractStream implements HeaderEmitter {
se.getError()));
}
state.sendReset();
+ cancelAllocationRequests();
handler.sendStreamReset(se);
} catch (IOException ioe) {
ConnectionException ce = new ConnectionException(
diff --git a/java/org/apache/coyote/http2/WindowAllocationManager.java b/java/org/apache/coyote/http2/WindowAllocationManager.java
index f286430..973ce59 100644
--- a/java/org/apache/coyote/http2/WindowAllocationManager.java
+++ b/java/org/apache/coyote/http2/WindowAllocationManager.java
@@ -17,6 +17,7 @@
package org.apache.coyote.http2;
import org.apache.coyote.ActionCode;
+import org.apache.coyote.Response;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.res.StringManager;
@@ -184,24 +185,27 @@ class WindowAllocationManager {
// to stream.notify(). Additional notify() calls may trigger
// unexpected timeouts.
waitingFor = NONE;
- if (stream.getCoyoteResponse().getWriteListener() == null) {
- // Blocking, so use notify to release StreamOutputBuffer
- if (log.isDebugEnabled()) {
- log.debug(sm.getString("windowAllocationManager.notified",
- stream.getConnectionId(), stream.getIdentifier()));
+ Response response = stream.getCoyoteResponse();
+ if (response != null) {
+ if (response.getWriteListener() == null) {
+ // Blocking, so use notify to release StreamOutputBuffer
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("windowAllocationManager.notified",
+ stream.getConnectionId(), stream.getIdentifier()));
+ }
+ stream.notify();
+ } else {
+ // Non-blocking so dispatch
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("windowAllocationManager.dispatched",
+ stream.getConnectionId(), stream.getIdentifier()));
+ }
+ response.action(ActionCode.DISPATCH_WRITE, null);
+ // Need to explicitly execute dispatches on the StreamProcessor
+ // as this thread is being processed by an UpgradeProcessor
+ // which won't see this dispatch
+ response.action(ActionCode.DISPATCH_EXECUTE, null);
}
- stream.notify();
- } else {
- // Non-blocking so dispatch
- if (log.isDebugEnabled()) {
- log.debug(sm.getString("windowAllocationManager.dispatched",
- stream.getConnectionId(), stream.getIdentifier()));
- }
- stream.getCoyoteResponse().action(ActionCode.DISPATCH_WRITE, null);
- // Need to explicitly execute dispatches on the StreamProcessor
- // as this thread is being processed by an UpgradeProcessor
- // which won't see this dispatch
- stream.getCoyoteResponse().action(ActionCode.DISPATCH_EXECUTE, null);
}
}
}
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
index c9182c8..7f8448f 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
@@ -379,8 +379,7 @@ public class TestHttp2Section_5_1 extends Http2TestBase {
parser.readFrame(true);
Assert.assertTrue(output.getTrace(),
- output.getTrace().contains("5-RST-[" +
- Http2Error.REFUSED_STREAM.getCode() + "]"));
+ output.getTrace().contains("5-RST-[" + Http2Error.REFUSED_STREAM.getCode() + "]"));
output.clearTrace();
// Connection window is zero.
@@ -391,5 +390,6 @@ public class TestHttp2Section_5_1 extends Http2TestBase {
sendWindowUpdate(3, (1 << 31) - 1);
parser.readFrame(true);
+ Assert.assertEquals("3-RST-[" + Http2Error.FLOW_CONTROL_ERROR.getCode() + "]\n", output.getTrace());
}
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 8a8ef8e..fb57417 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -104,6 +104,11 @@
closed error if errors occur after a stream has been reset by the
client. (markt)
</fix>
+ <fix>
+ Improve handling of HTTP/2 stream level flow control errors and notidy
+ the stream immediately if it is waiting for an allocation when the flow
+ control error occurs. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="WebSocket">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 01/03: Refactor for more consistent error code after
client sends stream reset
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 32c2e19d94546653e2cc18656d4df32d3a9bb1db
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Aug 18 20:24:53 2020 +0100
Refactor for more consistent error code after client sends stream reset
---
java/org/apache/coyote/http2/Http2UpgradeHandler.java | 7 ++-----
test/org/apache/coyote/http2/TestHttp2Section_5_1.java | 2 +-
webapps/docs/changelog.xml | 5 +++++
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 08ff5e7..62e1423 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -38,7 +38,6 @@ import java.util.concurrent.atomic.AtomicReference;
import javax.servlet.http.WebConnection;
import org.apache.coyote.Adapter;
-import org.apache.coyote.CloseNowException;
import org.apache.coyote.ProtocolException;
import org.apache.coyote.Request;
import org.apache.coyote.http11.upgrade.InternalHttpUpgradeHandler;
@@ -836,9 +835,7 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
do {
synchronized (this) {
if (!stream.canWrite()) {
- throw new CloseNowException(
- sm.getString("upgradeHandler.stream.notWritable",
- stream.getConnectionId(), stream.getIdentifier()));
+ stream.doStreamCancel(sm.getString("upgradeHandler.stream.notWritable"), Http2Error.STREAM_CLOSED);
}
long windowSize = getWindowSize();
if (windowSize < 1 || backLogSize > 0) {
@@ -910,7 +907,7 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
error = Http2Error.ENHANCE_YOUR_CALM;
} else {
msg = sm.getString("stream.clientCancel");
- error = Http2Error.CANCEL;
+ error = Http2Error.STREAM_CLOSED;
}
// Close the stream
// This thread is in application code so need
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
index 7bf83dd..c9182c8 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
@@ -321,7 +321,7 @@ public class TestHttp2Section_5_1 extends Http2TestBase {
// Client reset triggers a write error which in turn triggers a server
// reset
parser.readFrame(true);
- Assert.assertEquals("3-RST-[8]\n", output.getTrace());
+ Assert.assertEquals("3-RST-[5]\n", output.getTrace());
output.clearTrace();
// Open up the connection window.
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index c703e00..8a8ef8e 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -99,6 +99,11 @@
in the previous release to reduce the memory footprint of closed HTTP/2
streams. (markt)
</fix>
+ <fix>
+ Refactor the HTTP/2 implementation to more consistently return a stream
+ closed error if errors occur after a stream has been reset by the
+ client. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="WebSocket">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 03/03: Expand to ensure clean closure with connection flow
control error
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 d56541ffad0d2b12cdd41a413d7f3e67edc65223
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Aug 18 21:12:46 2020 +0100
Expand to ensure clean closure with connection flow control error
---
.../apache/coyote/http2/TestHttp2Section_5_1.java | 63 +++++++++++++++++++++-
1 file changed, 62 insertions(+), 1 deletion(-)
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
index 7f8448f..a9c3635 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
@@ -339,7 +339,7 @@ public class TestHttp2Section_5_1 extends Http2TestBase {
@Test
- public void testErrorOnWaitingStream() throws Exception {
+ public void testErrorOnWaitingStream01() throws Exception {
// http2Connect() - modified
enableHttp2(1);
configureAndStartWebApplication();
@@ -392,4 +392,65 @@ public class TestHttp2Section_5_1 extends Http2TestBase {
parser.readFrame(true);
Assert.assertEquals("3-RST-[" + Http2Error.FLOW_CONTROL_ERROR.getCode() + "]\n", output.getTrace());
}
+
+
+ @Test
+ public void testErrorOnWaitingStream02() throws Exception {
+ // http2Connect() - modified
+ enableHttp2(1);
+ configureAndStartWebApplication();
+ openClientConnection();
+ doHttpUpgrade();
+ sendClientPreface();
+
+ // validateHttp2InitialResponse() - modified
+ parser.readFrame(true);
+ parser.readFrame(true);
+ parser.readFrame(true);
+ parser.readFrame(true);
+ parser.readFrame(true);
+
+ Assert.assertEquals("0-Settings-[3]-[1]\n" +
+ "0-Settings-End\n" +
+ "0-Settings-Ack\n" +
+ "0-Ping-[0,0,0,0,0,0,0,1]\n" +
+ getSimpleResponseTrace(1)
+ , output.getTrace());
+ output.clearTrace();
+
+ sendLargeGetRequest(3);
+
+ sendSimpleGetRequest(5);
+
+ // Default connection window size is 64k-1.
+ // Initial request will have used 8k leaving 56k-1.
+ // Stream window will be 64k-1.
+
+ // Increase Connection window by 16k
+ sendWindowUpdate(0, 16 * 1024);
+
+ // Expecting
+ // 1 * headers
+ // 64k-1 of body (8 * ~8k)
+ // 1 * error (could be in any order)
+ for (int i = 0; i < 9; i++) {
+ parser.readFrame(true);
+ }
+ parser.readFrame(true);
+
+ Assert.assertTrue(output.getTrace(),
+ output.getTrace().contains("5-RST-[" + Http2Error.REFUSED_STREAM.getCode() + "]"));
+ output.clearTrace();
+
+ // Connection window is 8k.
+ // Stream window is zero.
+
+ // Expand the connection window too much to trigger an error
+ // Allow for the 8k still in the connection window
+ sendWindowUpdate(0, (1 << 31) - 1);
+
+ parser.readFrame(true);
+ Assert.assertTrue(output.getTrace(),
+ output.getTrace().contains("0-Goaway-[5]-[" + Http2Error.FLOW_CONTROL_ERROR.getCode() + "]"));
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org