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 ca4f298 Improve fix to avoid deadlock reported on some systems
ca4f298 is described below
commit ca4f2989465f4ead1e24ebccda0ab71379164798
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Jan 3 17:24:59 2022 +0000
Improve fix to avoid deadlock reported on some systems
https://markmail.org/message/ldpjfdwpkmqc7ved
---
java/org/apache/coyote/http2/Http2UpgradeHandler.java | 14 ++++++++++++--
java/org/apache/coyote/http2/Stream.java | 18 ++++++++++--------
webapps/docs/changelog.xml | 5 +++++
3 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index fcb05b0..a075c02 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -353,7 +353,7 @@ class Http2UpgradeHandler extends AbstractStream implements
InternalHttpUpgradeH
// continue reading frames
Stream stream = getStream(se.getStreamId(), false);
if (stream == null) {
- sendStreamReset(se);
+ sendStreamReset(null, se);
} else {
stream.close(se);
}
@@ -536,7 +536,7 @@ class Http2UpgradeHandler extends AbstractStream implements
InternalHttpUpgradeH
}
- void sendStreamReset(StreamException se) throws IOException {
+ void sendStreamReset(StreamStateMachine state, StreamException se) throws
IOException {
if (log.isDebugEnabled()) {
log.debug(sm.getString("upgradeHandler.rst.debug", connectionId,
@@ -555,7 +555,17 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
// Payload
ByteUtil.setFourBytes(rstFrame, 9, se.getError().getCode());
+ // Need to update state atomically with the sending of the RST
+ // frame else other threads currently working with this stream
+ // may see the state change and send a RST frame before the RST
+ // frame triggered by this thread. If that happens the client
+ // may see out of order RST frames which may hard to follow if
+ // the client is unaware the RST frames may be received out of
+ // order.
synchronized (socketWrapper) {
+ if (state != null) {
+ state.sendReset();
+ }
socketWrapper.write(true, rstFrame, 0, rstFrame.length);
socketWrapper.flush(true);
}
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index ac3dd72..8fee970 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -660,14 +660,16 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
log.debug(sm.getString("stream.reset.send",
getConnectionId(), getIdAsString(),
se.getError()));
}
- // Sync ensures that if the call to sendReset() triggers resets
- // in other threads, that the RST frame associated with this
- // thread is sent before the RST frames associated with those
- // threads.
- synchronized (state) {
- state.sendReset();
- handler.sendStreamReset(se);
- }
+
+ // Need to update state atomically with the sending of the RST
+ // frame else other threads currently working with this stream
+ // may see the state change and send a RST frame before the RST
+ // frame triggered by this thread. If that happens the client
+ // may see out of order RST frames which may hard to follow if
+ // the client is unaware the RST frames may be received out of
+ // order.
+ handler.sendStreamReset(state, se);
+
cancelAllocationRequests();
if (inputBuffer != null) {
inputBuffer.swallowUnread();
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index e1b858d..7d95f41 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -141,6 +141,11 @@
<bug>65757</bug>: Missing initial IO listener notification on Servlet
container dispatch to another container thread. (remm)
</fix>
+ <fix>
+ Improve the fix for RST frame ordering added in 10.1.0-M8 to avoid a
+ potential deadlock on some systems in non-default configurations.
+ (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
