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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org