Author: kkolinko Date: Sun Oct 25 18:26:23 2015 New Revision: 1710473 URL: http://svn.apache.org/viewvc?rev=1710473&view=rev Log: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57943 Prevent the same socket being added to the cache twice. Patch based on analysis by Ian Luo / Sun Qi. http://svn.apache.org/r1688909 http://svn.apache.org/r1688911
CTR: The backport also includes changes from the following commits: http://svn.apache.org/r1302839 - Adding (attachment == null) check in processSocket(). This suppresses a log message described in BZ 52926. A trivial fix. As the whole method is wrapped by try/catch(Throwable) the only effect from this is to skip logging a message. http://svn.apache.org/r1646466 http://svn.apache.org/r1646468 - Changing cancelledKey() method to return KeyAttachment if cancelling was successful and null if not (e.g. canceled by another thread) http://svn.apache.org/r1710467 - Trivial followup to r1688909 removing unneeded code. This was part of original fix in Tomcat 8. Modified: tomcat/tc6.0.x/trunk/STATUS.txt tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Modified: tomcat/tc6.0.x/trunk/STATUS.txt URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1710473&r1=1710472&r2=1710473&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/STATUS.txt (original) +++ tomcat/tc6.0.x/trunk/STATUS.txt Sun Oct 25 18:26:23 2015 @@ -28,16 +28,6 @@ None PATCHES PROPOSED TO BACKPORT: [ New proposals should be added at the end of the list ] -* Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57943 - Prevent the same socket being added to the cache twice. Patch based on - analysis by Ian Luo / Sun Qi. - http://svn.apache.org/r1688909 - http://svn.apache.org/r1688911 - +1: markt, remm, kkolinko - -1: - kkolinko: Technically, this also includes http://svn.apache.org/r1646468 - (adding return value to cancelledKey() method, the rest is superseded by r1688909) - * Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=58031 Provide a mechanism to enable a 413 response if maxPostSize is exceeded while processing parameters Modified: tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java?rev=1710473&r1=1710472&r2=1710473&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java (original) +++ tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java Sun Oct 25 18:26:23 2015 @@ -1269,6 +1269,9 @@ public class NioEndpoint extends Abstrac protected boolean processSocket(NioChannel socket, SocketStatus status, boolean dispatch) { try { KeyAttachment attachment = (KeyAttachment)socket.getAttachment(false); + if (attachment == null) { + return false; + } attachment.setCometNotify(false); //will get reset upon next reg if (executor == null) { getWorkerThread().assign(socket, status); @@ -1536,23 +1539,25 @@ public class NioEndpoint extends Abstrac else r.reset(socket,ka,OP_REGISTER); addEvent(r); } - public void cancelledKey(SelectionKey key, SocketStatus status, boolean dispatch) { + public KeyAttachment cancelledKey(SelectionKey key, SocketStatus status, boolean dispatch) { + KeyAttachment ka = null; try { - if ( key == null ) return;//nothing to do - KeyAttachment ka = (KeyAttachment) key.attachment(); + if ( key == null ) return null;//nothing to do + ka = (KeyAttachment) key.attachment(); if (ka != null && ka.getComet() && status != null) { //the comet event takes care of clean up //processSocket(ka.getChannel(), status, dispatch); ka.setComet(false);//to avoid a loop if (status == SocketStatus.TIMEOUT ) { - processSocket(ka.getChannel(), status, true); - return; // don't close on comet timeout + if (processSocket(ka.getChannel(), status, true)) { + return null; // don't close on comet timeout + } } else { - processSocket(ka.getChannel(), status, false); //don't dispatch if the lines below are cancelling the key + // Don't dispatch if the lines below are cancelling the key + processSocket(ka.getChannel(), status, false); } } - - key.attach(null); + ka = (KeyAttachment) key.attach(null); if (ka!=null) handler.release(ka.getChannel()); if (key.isValid()) key.cancel(); if (key.channel().isOpen()) { @@ -1578,6 +1583,7 @@ public class NioEndpoint extends Abstrac if ( log.isDebugEnabled() ) log.error("",e); // Ignore } + return ka; } /** * The background thread that listens for incoming TCP/IP connections and @@ -2307,31 +2313,18 @@ public class NioEndpoint extends Abstrac if (closed) { // Close socket and pool - try { - KeyAttachment ka = null; - if (key!=null) { - ka = (KeyAttachment) key.attachment(); - if (ka!=null) ka.setComet(false); - socket.getPoller().cancelledKey(key, SocketStatus.ERROR, false); - } - if (socket!=null) nioChannels.offer(socket); - socket = null; - if ( ka!=null ) keyCache.offer(ka); - ka = null; - }catch ( Exception x ) { - log.error("",x); + KeyAttachment ka = null; + if (key!=null) { + ka = (KeyAttachment) key.attachment(); } + close(ka, socket, key, SocketStatus.ERROR); } } else if (handshake == -1 ) { KeyAttachment ka = null; if (key!=null) { ka = (KeyAttachment) key.attachment(); - socket.getPoller().cancelledKey(key, SocketStatus.DISCONNECT, false); } - if (socket!=null) nioChannels.offer(socket); - socket = null; - if ( ka!=null ) keyCache.offer(ka); - ka = null; + close(ka, socket, key, SocketStatus.DISCONNECT); } else { final SelectionKey fk = key; final int intops = handshake; @@ -2364,6 +2357,31 @@ public class NioEndpoint extends Abstrac } } } + + private void close(KeyAttachment ka, NioChannel socket, SelectionKey key, + SocketStatus socketStatus) { + try { + if (ka != null) { + ka.setComet(false); + } + if (socket.getPoller().cancelledKey(key, socketStatus, false) != null) { + // SocketWrapper (attachment) was removed from the + // key - recycle both. This can only happen once + // per attempted closure so it is used to determine + // whether or not to return socket and ka to + // their respective caches. We do NOT want to do + // this more than once - see BZ 57340 / 57943. + if (running && !paused) { + nioChannels.offer(socket); + } + if (running && !paused && ka != null) { + keyCache.offer(ka); + } + } + } catch ( Exception x ) { + log.error("",x); + } + } } // ---------------------------------------------- TaskQueue Inner Class Modified: tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml?rev=1710473&r1=1710472&r2=1710473&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml (original) +++ tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Sun Oct 25 18:26:23 2015 @@ -67,6 +67,15 @@ Align the Java side of the tc-native connector with the Tomcat 7 implementation to ease future maintenance. (markt) </add> + <fix> + <bug>52926</bug>: Avoid NPE when an NIO Comet connection times out on + one thread at the same time as it is closed on another thread. + (markt/kkolinko) + </fix> + <fix> + <bug>57943</bug>: Prevent the same socket being added to the cache + twice. Patch based on analysis by Ian Luo / Sun Qi. (markt/kkolinko) + </fix> </changelog> </subsection> <subsection name="Web applications"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
