svn commit: r1799190 - in /tomcat/trunk: java/org/apache/tomcat/websocket/WsSession.java webapps/docs/changelog.xml

[ma...@apache.org]

Author: markt
Date: Mon Jun 19 12:24:56 2017
New Revision: 1799190

URL: http://svn.apache.org/viewvc?rev=1799190&view=rev
Log:
Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=61183
Correct a regression in the previous fix for BZ 58624 that could trigger a deadlock depending on the locking strategy employed by the client code.

Modified:
    tomcat/trunk/java/org/apache/tomcat/websocket/WsSession.java
    tomcat/trunk/webapps/docs/changelog.xml

Modified: tomcat/trunk/java/org/apache/tomcat/websocket/WsSession.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/WsSession.java?rev=1799190&r1=1799189&r2=1799190&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/websocket/WsSession.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/websocket/WsSession.java Mon Jun 19 12:24:56 2017
@@ -648,28 +648,48 @@ public class WsSession implements Sessio
      * @param f2sh The handler
      */
     protected void registerFuture(FutureToSendHandler f2sh) {
-        boolean fail = false;
-        synchronized (stateLock) {
-            // If the session has already been closed the any registered futures
-            // will have been processed so the failure result for this future
-            // needs to be set here.
-            if (state == State.OPEN) {
-                futures.put(f2sh, f2sh);
-            } else if (f2sh.isDone()) {
-                // NO-OP. The future completed before the session closed so no
-                // need to register in case the session closes before it
-                // completes.
-            } else {
-                // Construct the exception outside of the sync block
-                fail = true;
-            }
+        // Ideally, this code should sync on stateLock so that the correct
+        // action is taken based on the current state of the connection.
+        // However, a sync on stateLock can't be used here as it will create the
+        // possibility of a dead-lock. See BZ 61183.
+        // Therefore, a slightly less efficient approach is used.
+
+        // Always register the future.
+        futures.put(f2sh, f2sh);
+
+        if (state == State.OPEN) {
+            // The session is open. The future has been registered with the open
+            // session. Normal processing continues.
+            return;
         }
 
-        if (fail) {
-            IOException ioe = new IOException(sm.getString("wsSession.messageFailed"));
-            SendResult sr = new SendResult(ioe);
-            f2sh.onResult(sr);
+        // The session is closed. The future may or may not have been registered
+        // in time for it to be processed during session closure.
+
+        if (f2sh.isDone()) {
+            // The future has completed. It is not known if the future was
+            // completed normally by the I/O layer or in error by doClose(). It
+            // doesn't matter which. There is nothing more to do here.
+            return;
         }
+
+        // The session is closed. The Future had not completed when last checked.
+        // There is a small timing window that means the Future may have been
+        // completed since the last check. There is also the possibility that
+        // the Future was not registered in time to be cleaned up during session
+        // close.
+        // Attempt to complete the Future with an error result as this ensures
+        // that the Future completes and any client code waiting on it does not
+        // hang. It is slightly inefficient since the Future may have been
+        // completed in another thread or another thread may be about to
+        // complete the Future but knowing if this is the case requires the sync
+        // on stateLock (see above).
+        // Note: If multiple attempts are made to complete the Future, the
+        //       second and subsequent attempts are ignored.
+
+        IOException ioe = new IOException(sm.getString("wsSession.messageFailed"));
+        SendResult sr = new SendResult(ioe);
+        f2sh.onResult(sr);
     }
 
 

Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1799190&r1=1799189&r2=1799190&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Mon Jun 19 12:24:56 2017
@@ -201,6 +201,11 @@
         and/or stack traces on web application stop and/or start when running
         under a security manager. (markt)
       </fix>
+      <fix>
+        <bug>61183</bug>: Correct a regression in the previous fix for
+        <bug>58624</bug> that could trigger a deadlock depending on the locking
+        strategy employed by the client code. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Other">



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