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
The following commit(s) were added to refs/heads/8.5.x by this push:
new 5e5d14e772 Fix race condition that was causing intermittent CI failures
5e5d14e772 is described below
commit 5e5d14e7729fdea7dc5faa439b48fc18740370e1
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 2962c0699e..d8a3ce1b6c 100644
--- a/java/org/apache/coyote/http2/Http2Parser.java
+++ b/java/org/apache/coyote/http2/Http2Parser.java
@@ -581,10 +581,9 @@ class Http2Parser {
// going to be thrown.
hpackDecoder.getHeaderEmitter().validateHeaders();
- output.headersEnd(streamId);
+ output.headersEnd(streamId, headersEndStream);
if (headersEndStream) {
- output.receivedEndOfStream(streamId);
headersEndStream = false;
}
@@ -720,7 +719,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 925e43e0d7..b5e5d5ce08 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1652,20 +1652,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);
@@ -1753,10 +1739,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()) {
@@ -1774,9 +1761,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 b929715650..518a174c89 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -1082,13 +1082,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);
@@ -1137,8 +1130,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 e689d18d40..14854dcab1 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -143,6 +143,10 @@
called after an error during asynchronous processing with HTTP/2.
(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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
