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 2023/08/07 16:14:36 UTC
[tomcat] branch 9.0.x updated: Fix race condition that was causing intermittent CI failures
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new c7fb31ccb0 Fix race condition that was causing intermittent CI failures
c7fb31ccb0 is described below
commit c7fb31ccb0a1154ac7ad2d7e4696e2b1cf226796
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Aug 7 17:13:55 2023 +0100
Fix race condition that was causing intermittent CI failures
Ensure that, if there is no request body, the stream is notified that
end-of-stream has been received before passing processing of the request
to a container thread.
---
java/org/apache/coyote/http2/Http2Parser.java | 5 +--
.../apache/coyote/http2/Http2UpgradeHandler.java | 50 +++++++++++++++-------
test/org/apache/coyote/http2/Http2TestBase.java | 19 ++++----
webapps/docs/changelog.xml | 4 ++
4 files changed, 51 insertions(+), 27 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2Parser.java b/java/org/apache/coyote/http2/Http2Parser.java
index da61589c93..ee84cc17c8 100644
--- a/java/org/apache/coyote/http2/Http2Parser.java
+++ b/java/org/apache/coyote/http2/Http2Parser.java
@@ -643,10 +643,9 @@ class Http2Parser {
hpackDecoder.getHeaderEmitter().validateHeaders();
synchronized (output) {
- output.headersEnd(streamId);
+ output.headersEnd(streamId, headersEndStream);
if (headersEndStream) {
- output.receivedEndOfStream(streamId);
headersEndStream = false;
}
}
@@ -794,7 +793,7 @@ class Http2Parser {
void headersContinue(int payloadSize, boolean endOfHeaders);
- void headersEnd(int streamId) throws Http2Exception;
+ void headersEnd(int streamId, boolean endOfStream) throws Http2Exception;
// Reset frames
void reset(int streamId, long errorCode) throws Http2Exception;
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index d8e04d04da..22472e87d5 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1541,20 +1541,6 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
}
- @Override
- public void receivedEndOfStream(int streamId) throws ConnectionException {
- AbstractNonZeroStream abstractNonZeroStream =
- getAbstractNonZeroStream(streamId, connectionState.get().isNewStreamAllowed());
- if (abstractNonZeroStream instanceof Stream) {
- Stream stream = (Stream) abstractNonZeroStream;
- stream.receivedEndOfStream();
- if (!stream.isActive()) {
- setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
- }
- }
- }
-
-
@Override
public void onSwallowedDataFramePayload(int streamId, int swallowedDataBytesCount) throws IOException {
AbstractNonZeroStream abstractNonZeroStream = getAbstractNonZeroStream(streamId);
@@ -1642,10 +1628,11 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
@Override
- public void headersEnd(int streamId) throws Http2Exception {
+ public void headersEnd(int streamId, boolean endOfStream) throws Http2Exception {
AbstractNonZeroStream abstractNonZeroStream =
getAbstractNonZeroStream(streamId, connectionState.get().isNewStreamAllowed());
if (abstractNonZeroStream instanceof Stream) {
+ boolean processStream = false;
setMaxProcessedStream(streamId);
Stream stream = (Stream) abstractNonZeroStream;
if (stream.isActive()) {
@@ -1663,9 +1650,40 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
// Valid new stream reduces the overhead count
reduceOverheadCount(FrameType.HEADERS);
- processStreamOnContainerThread(stream);
+ processStream = true;
}
}
+ /*
+ * Need to process end of stream before calling processStreamOnContainerThread to avoid a race condition
+ * where the container thread finishes before end of stream is processed, thinks the request hasn't been
+ * fully read so issues a RST with error code 0 (NO_ERROR) to tell the client not to send the request body,
+ * if any. This breaks tests and generates unnecessary RST messages for standard clients.
+ */
+ if (endOfStream) {
+ receivedEndOfStream(stream);
+ }
+ if (processStream) {
+ processStreamOnContainerThread(stream);
+ }
+ }
+ }
+
+
+ @Override
+ public void receivedEndOfStream(int streamId) throws ConnectionException {
+ AbstractNonZeroStream abstractNonZeroStream =
+ getAbstractNonZeroStream(streamId, connectionState.get().isNewStreamAllowed());
+ if (abstractNonZeroStream instanceof Stream) {
+ Stream stream = (Stream) abstractNonZeroStream;
+ receivedEndOfStream(stream);
+ }
+ }
+
+
+ private void receivedEndOfStream(Stream stream) throws ConnectionException {
+ stream.receivedEndOfStream();
+ if (!stream.isActive()) {
+ setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
}
}
diff --git a/test/org/apache/coyote/http2/Http2TestBase.java b/test/org/apache/coyote/http2/Http2TestBase.java
index 7333cec848..73c5734b85 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -1087,13 +1087,6 @@ public abstract class Http2TestBase extends TomcatBaseTest {
}
- @Override
- public void receivedEndOfStream(int streamId) {
- lastStreamId = Integer.toString(streamId);
- trace.append(lastStreamId + "-EndOfStream\n");
- }
-
-
@Override
public HeaderEmitter headersStart(int streamId, boolean headersEndStream) {
lastStreamId = Integer.toString(streamId);
@@ -1142,8 +1135,18 @@ public abstract class Http2TestBase extends TomcatBaseTest {
@Override
- public void headersEnd(int streamId) {
+ public void headersEnd(int streamId, boolean endOfStream) {
trace.append(streamId + "-HeadersEnd\n");
+ if (endOfStream) {
+ receivedEndOfStream(streamId) ;
+ }
+ }
+
+
+ @Override
+ public void receivedEndOfStream(int streamId) {
+ lastStreamId = Integer.toString(streamId);
+ trace.append(lastStreamId + "-EndOfStream\n");
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index a8e6e3c224..832a9c63c0 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -170,6 +170,10 @@
NIO2), include DATA frames when calculating the HTTP/2 overhead count to
ensure that connections are not prematurely terminated. (markt)
</fix>
+ <fix>
+ Correct a race condition that could cause spurious RST messages to be
+ sent after the response had been written to an HTTP/2 stream. (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