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 6285e75  Improve fix to avoid deadlock reported on some systems
6285e75 is described below

commit 6285e750e2b640fa9c59c11c09a753a36a5396b8
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 ++++++++++--------
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java 
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 1195997..a2c87ed 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -345,7 +345,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);
                             }
@@ -535,7 +535,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,
@@ -554,7 +554,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 fffd240..a4d6484 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -614,14 +614,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();

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to