This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new d3f3359d2b Fix race condition that was causing intermittent CI failures
d3f3359d2b is described below
commit d3f3359d2ba3abdd8b9719ab9e3c45c51610305d
Author: Mark Thomas <[email protected]>
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 1eb0def3f6..c5f7f16ad5 100644
--- a/java/org/apache/coyote/http2/Http2Parser.java
+++ b/java/org/apache/coyote/http2/Http2Parser.java
@@ -626,10 +626,9 @@ class Http2Parser {
hpackDecoder.getHeaderEmitter().validateHeaders();
synchronized (output) {
- output.headersEnd(streamId);
+ output.headersEnd(streamId, headersEndStream);
if (headersEndStream) {
- output.receivedEndOfStream(streamId);
headersEndStream = false;
}
}
@@ -777,7 +776,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 8b6c53bad5..389c2273a5 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1547,20 +1547,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);
@@ -1631,10 +1617,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()) {
@@ -1652,9 +1639,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 2c8ff99c0c..7356f51b07 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -1118,13 +1118,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);
@@ -1173,8 +1166,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 f3515ba866..e0d84c46fc 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -170,6 +170,10 @@
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: [email protected]
For additional commands, e-mail: [email protected]
