You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2020/11/11 20:49:49 UTC
[tomcat] branch 9.0.x updated: Fix NIO concurrency issue that
removes connections from the poller.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org