This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push: new ef17d67 Fix NIO concurrency issue that removes connections from the poller. ef17d67 is described below commit ef17d67acb752c843f330a0ee3525bf71f7396b1 Author: Mark Thomas <ma...@apache.org> AuthorDate: Wed Nov 11 20:43:04 2020 +0000 Fix NIO concurrency issue that removes connections from the poller. This is the source of the intermittent WebSocket test failure so this commit also removes the associated debug code for that issue. --- java/org/apache/tomcat/util/net/NioEndpoint.java | 31 +++++++++++++++++----- .../tomcat/websocket/TestWebSocketFrameClient.java | 24 ++++------------- webapps/docs/changelog.xml | 4 +++ 3 files changed, 33 insertions(+), 26 deletions(-) diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java b/java/org/apache/tomcat/util/net/NioEndpoint.java index d960d39..97e9f6d 100644 --- a/java/org/apache/tomcat/util/net/NioEndpoint.java +++ b/java/org/apache/tomcat/util/net/NioEndpoint.java @@ -1549,7 +1549,14 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel> @Override protected void doRun() { - NioChannel socket = socketWrapper.getSocket(); + /* + * Do not cache and re-use the value of socketWrapper.getSocket() in + * this method. If the socket closes the value will be updated to + * CLOSED_NIO_CHANNEL and the previous value potentially re-used for + * a new connection. That can result in a stale cached value which + * in turn can result in unintentionally closing currently active + * connections. + */ Poller poller = NioEndpoint.this.poller; if (poller == null) { socketWrapper.close(); @@ -1559,7 +1566,7 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel> try { int handshake = -1; try { - if (socket.isHandshakeComplete()) { + if (socketWrapper.getSocket().isHandshakeComplete()) { // No TLS handshaking required. Let the handler // process this socket / event combination. handshake = 0; @@ -1569,7 +1576,7 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel> // if the handshake failed. handshake = -1; } else { - handshake = socket.handshake(event == SocketEvent.OPEN_READ, event == SocketEvent.OPEN_WRITE); + handshake = socketWrapper.getSocket().handshake(event == SocketEvent.OPEN_READ, event == SocketEvent.OPEN_WRITE); // The handshake process reads/writes from/to the // socket. status may therefore be OPEN_WRITE once // the handshake completes. However, the handshake @@ -1594,23 +1601,23 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel> state = getHandler().process(socketWrapper, event); } if (state == SocketState.CLOSED) { - poller.cancelledKey(socket.getIOChannel().keyFor(poller.getSelector()), socketWrapper); + poller.cancelledKey(getSelectionKey(), socketWrapper); } } else if (handshake == -1 ) { getHandler().process(socketWrapper, SocketEvent.CONNECT_FAIL); - poller.cancelledKey(socket.getIOChannel().keyFor(poller.getSelector()), socketWrapper); + poller.cancelledKey(getSelectionKey(), socketWrapper); } else if (handshake == SelectionKey.OP_READ){ socketWrapper.registerReadInterest(); } else if (handshake == SelectionKey.OP_WRITE){ socketWrapper.registerWriteInterest(); } } catch (CancelledKeyException cx) { - poller.cancelledKey(socket.getIOChannel().keyFor(poller.getSelector()), socketWrapper); + poller.cancelledKey(getSelectionKey(), socketWrapper); } catch (VirtualMachineError vme) { ExceptionUtils.handleThrowable(vme); } catch (Throwable t) { log.error(sm.getString("endpoint.processing.fail"), t); - poller.cancelledKey(socket.getIOChannel().keyFor(poller.getSelector()), socketWrapper); + poller.cancelledKey(getSelectionKey(), socketWrapper); } finally { socketWrapper = null; event = null; @@ -1620,8 +1627,18 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel> } } } + + private SelectionKey getSelectionKey() { + SocketChannel socketChannel = socketWrapper.getSocket().getIOChannel(); + if (socketChannel == null) { + return null; + } + + return socketChannel.keyFor(NioEndpoint.this.poller.getSelector()); + } } + // ----------------------------------------------- SendfileData Inner Class /** diff --git a/test/org/apache/tomcat/websocket/TestWebSocketFrameClient.java b/test/org/apache/tomcat/websocket/TestWebSocketFrameClient.java index 9f5c730..ee67b49 100644 --- a/test/org/apache/tomcat/websocket/TestWebSocketFrameClient.java +++ b/test/org/apache/tomcat/websocket/TestWebSocketFrameClient.java @@ -23,8 +23,6 @@ import java.util.Map; import java.util.Queue; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; -import java.util.logging.Level; -import java.util.logging.LogManager; import javax.websocket.ClientEndpointConfig; import javax.websocket.ClientEndpointConfig.Configurator; @@ -117,20 +115,11 @@ public class TestWebSocketFrameClient extends WebSocketBaseTest { tomcat.start(); - LogManager.getLogManager().getLogger("org.apache.coyote").setLevel(Level.ALL); - LogManager.getLogManager().getLogger("org.apache.tomcat.websocket").setLevel(Level.ALL); - LogManager.getLogManager().getLogger("org.apache.tomcat.util.net").setLevel(Level.ALL); - try { - echoTester("",null); - echoTester("/",null); - // This will trigger a redirect so there will be 5 requests logged - echoTester("/foo",null); - echoTester("/foo/",null); - } finally { - LogManager.getLogManager().getLogger("org.apache.coyote").setLevel(Level.INFO); - LogManager.getLogManager().getLogger("org.apache.tomcat.websocket.WsWebSocketContainer").setLevel(Level.INFO); - LogManager.getLogManager().getLogger("org.apache.tomcat.util.net").setLevel(Level.INFO); - } + echoTester("",null); + echoTester("/",null); + // This will trigger a redirect so there will be 5 requests logged + echoTester("/foo",null); + echoTester("/foo/",null); } public void echoTester(String path, ClientEndpointConfig clientEndpointConfig) @@ -198,7 +187,6 @@ public class TestWebSocketFrameClient extends WebSocketBaseTest { clientEndpointConfig.getUserProperties().put(Constants.WS_AUTHENTICATION_PASSWORD, utf8Pass); echoTester(URI_PROTECTED, clientEndpointConfig); - } @Test @@ -235,7 +223,5 @@ public class TestWebSocketFrameClient extends WebSocketBaseTest { clientEndpointConfig.getUserProperties().put(Constants.WS_AUTHENTICATION_PASSWORD,PWD); echoTester(URI_PROTECTED, clientEndpointConfig); - } - } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 84c09d0..8617401 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -140,6 +140,10 @@ <fix> <bug>64830</bug>: Fix concurrency issue in HPACK decoder. (markt) </fix> + <fix> + Fix a concurrency issue in the NIO connector that could cause newly + created connections to be removed from the poller. (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