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 <[email protected]>
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: [email protected]
For additional commands, e-mail: [email protected]
