You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2019/11/19 22:19:13 UTC
[tomcat] branch master updated: Refactor endpoint
close/destroySocket API
This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push:
new a8388e7 Refactor endpoint close/destroySocket API
a8388e7 is described below
commit a8388e72ec1fe27d55d7b5a2b3aa01032ebb18d5
Author: remm <re...@apache.org>
AuthorDate: Tue Nov 19 23:18:57 2019 +0100
Refactor endpoint close/destroySocket API
These APIs exist mostly for APR, but is actually useful for cleanup for
errors which occur during setSocketOptions.
The API is refactored to the following behavior which likely corresponds
to what was originally intended:
- closeSocket: close the socket through the wrapper using the connection
to look it up in the map
- destroySocket: low level close when no wrapper exists, this happens
when accepting while the connector is not started, or setSocketOptions
has an unexpected error before the wrapper is created
For NIOx, ensure that setSocketOptions errors call destorySocket if an
error occurs before the wrapper is associated with the connection
(closeSocket will do nothing in that case).
For APR, refactor to use the wrapper close in all cases (except the use
in the acceptor, like for the other connectors), the poller will then
call the low level destroy once that close is done.
---
.../apache/tomcat/util/net/AbstractEndpoint.java | 26 ++++--
java/org/apache/tomcat/util/net/AprEndpoint.java | 96 +++++++++-------------
java/org/apache/tomcat/util/net/Nio2Endpoint.java | 30 ++++---
java/org/apache/tomcat/util/net/NioEndpoint.java | 38 ++++-----
.../apache/tomcat/util/net/SocketWrapperBase.java | 1 -
webapps/docs/changelog.xml | 8 ++
6 files changed, 100 insertions(+), 99 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/AbstractEndpoint.java b/java/org/apache/tomcat/util/net/AbstractEndpoint.java
index 8f216c4..12b8a25 100644
--- a/java/org/apache/tomcat/util/net/AbstractEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AbstractEndpoint.java
@@ -189,9 +189,9 @@ public abstract class AbstractEndpoint<S,U> {
private ObjectName oname = null;
/**
- * Map holding all current connections keyed with the sockets (for APR).
+ * Map holding all current connections keyed with the sockets.
*/
- protected Map<S, SocketWrapperBase<S>> connections = new ConcurrentHashMap<>();
+ protected Map<U, SocketWrapperBase<S>> connections = new ConcurrentHashMap<>();
/**
* Get a set with the current open connections.
@@ -1355,14 +1355,24 @@ public abstract class AbstractEndpoint<S,U> {
/**
* Close the socket when the connection has to be immediately closed when
- * an error occurs while configuring the accepted socket, allocating
- * a wrapper for the socket, or trying to dispatch it for processing.
+ * an error occurs while configuring the accepted socket or trying to
+ * dispatch it for processing. The wrapper associated with the socket will
+ * be used for the close.
* @param socket The newly accepted socket
*/
- protected abstract void closeSocket(U socket);
-
- protected void destroySocket(U socket) {
- closeSocket(socket);
+ protected void closeSocket(U socket) {
+ SocketWrapperBase<S> socketWrapper = connections.get(socket);
+ if (socketWrapper != null) {
+ socketWrapper.close();
+ }
}
+
+ /**
+ * Close the socket. This is used when the connector is not in a state
+ * which allows processing the socket, or if there was an error which
+ * prevented the allocation of the socket wrapper.
+ * @param socket The newly accepted socket
+ */
+ protected abstract void destroySocket(U socket);
}
diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java
index 146a816..31b811b 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -695,8 +695,6 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
// the pool and its queue are full
log.error(sm.getString("endpoint.process.fail"), t);
}
- // Note: doing connections.remove is not needed since closeSocket will
- // be called, which then calls close on the wrapper
return false;
}
@@ -744,43 +742,19 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
}
- @Override
- protected void closeSocket(Long socket) {
- closeSocket(socket.longValue());
- }
-
-
- private void closeSocket(long socket) {
- // Once this is called, the mapping from socket to wrapper will no
- // longer be required.
- SocketWrapperBase<Long> wrapper = connections.remove(Long.valueOf(socket));
- if (wrapper != null) {
- // Cast to avoid having to catch an IOE that is never thrown.
- ((AprSocketWrapper) wrapper).close();
- }
+ private void closeSocketInternal(long socket) {
+ closeSocket(Long.valueOf(socket));
}
- /*
- * This method should only be called if there is no chance that the socket
- * is currently being used by the Poller. It is generally a bad idea to call
- * this directly from a known error condition.
- */
@Override
protected void destroySocket(Long socket) {
- destroySocket(socket.longValue());
+ countDownConnection();
+ destroySocketInternal(socket.longValue());
}
-
- /*
- * This method should only be called if there is no chance that the socket
- * is currently being used by the Poller. It is generally a bad idea to call
- * this directly from a known error condition.
- */
- private void destroySocket(long socket) {
- closeSocket(socket);
+ private void destroySocketInternal(long socket) {
if (log.isDebugEnabled()) {
- String msg = sm.getString("endpoint.debug.destroySocket",
- Long.valueOf(socket));
+ String msg = sm.getString("endpoint.debug.destroySocket", Long.valueOf(socket));
if (log.isTraceEnabled()) {
log.trace(msg, new Exception());
} else {
@@ -1116,7 +1090,8 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
removeFromPoller(info.socket);
// Poller isn't running at this point so use destroySocket()
// directly
- destroySocket(info.socket);
+ closeSocketInternal(info.socket);
+ destroySocketInternal(info.socket);
info = closeList.get();
}
closeList.clear();
@@ -1127,7 +1102,8 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
removeFromPoller(info.socket);
// Poller isn't running at this point so use destroySocket()
// directly
- destroySocket(info.socket);
+ closeSocketInternal(info.socket);
+ destroySocketInternal(info.socket);
info = addList.get();
}
addList.clear();
@@ -1135,7 +1111,8 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
int rv = Poll.pollset(aprPoller, desc);
if (rv > 0) {
for (int n = 0; n < rv; n++) {
- destroySocket(desc[n*2+1]);
+ closeSocketInternal(desc[n*2+1]);
+ destroySocketInternal(desc[n*2+1]);
}
}
Pool.destroy(pool);
@@ -1358,7 +1335,8 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
while (info != null) {
localAddList.remove(info.socket);
removeFromPoller(info.socket);
- destroySocket(info.socket);
+ closeSocketInternal(info.socket);
+ destroySocketInternal(info.socket);
info = localCloseList.get();
}
}
@@ -1387,7 +1365,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
// the poller.
removeFromPoller(info.socket);
if (!addToPoller(info.socket, wrapper.pollerFlags)) {
- closeSocket(info.socket);
+ wrapper.close();
} else {
timeouts.add(info.socket,
System.currentTimeMillis() +
@@ -1395,7 +1373,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
}
} else {
// Should never happen.
- closeSocket(info.socket);
+ wrapper.close();
getLog().warn(sm.getString(
"endpoint.apr.pollAddInvalid", info));
}
@@ -1445,31 +1423,31 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
// Error probably occurred during a non-blocking read
if (!processSocket(desc[n*2+1], SocketEvent.OPEN_READ)) {
// Close socket and clear pool
- closeSocket(desc[n*2+1]);
+ wrapper.close();
}
} else if ((desc[n*2] & Poll.APR_POLLOUT) == Poll.APR_POLLOUT) {
// Error probably occurred during a non-blocking write
if (!processSocket(desc[n*2+1], SocketEvent.OPEN_WRITE)) {
// Close socket and clear pool
- closeSocket(desc[n*2+1]);
+ wrapper.close();
}
} else if ((wrapper.pollerFlags & Poll.APR_POLLIN) == Poll.APR_POLLIN) {
// Can't tell what was happening when the error occurred but the
// socket is registered for non-blocking read so use that
if (!processSocket(desc[n*2+1], SocketEvent.OPEN_READ)) {
// Close socket and clear pool
- closeSocket(desc[n*2+1]);
+ wrapper.close();
}
} else if ((wrapper.pollerFlags & Poll.APR_POLLOUT) == Poll.APR_POLLOUT) {
// Can't tell what was happening when the error occurred but the
// socket is registered for non-blocking write so use that
if (!processSocket(desc[n*2+1], SocketEvent.OPEN_WRITE)) {
// Close socket and clear pool
- closeSocket(desc[n*2+1]);
+ wrapper.close();
}
} else {
// Close socket and clear pool
- closeSocket(desc[n*2+1]);
+ wrapper.close();
}
} else if (((desc[n*2] & Poll.APR_POLLIN) == Poll.APR_POLLIN)
|| ((desc[n*2] & Poll.APR_POLLOUT) == Poll.APR_POLLOUT)) {
@@ -1478,14 +1456,14 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
!processSocket(desc[n*2+1], SocketEvent.OPEN_READ)) {
error = true;
// Close socket and clear pool
- closeSocket(desc[n*2+1]);
+ wrapper.close();
}
if (!error &&
((desc[n*2] & Poll.APR_POLLOUT) == Poll.APR_POLLOUT) &&
!processSocket(desc[n*2+1], SocketEvent.OPEN_WRITE)) {
// Close socket and clear pool
error = true;
- closeSocket(desc[n*2+1]);
+ wrapper.close();
}
if (!error && wrapper.pollerFlags != 0) {
// If socket was registered for multiple events but
@@ -1517,7 +1495,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
"endpoint.apr.pollUnknownEvent",
Long.valueOf(desc[n*2])));
// Close socket and clear pool
- closeSocket(desc[n*2+1]);
+ wrapper.close();
}
}
} else if (rv < 0) {
@@ -1686,13 +1664,13 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
// Close any socket remaining in the add queue
for (int i = (addS.size() - 1); i >= 0; i--) {
SendfileData data = addS.get(i);
- closeSocket(data.socket);
+ closeSocketInternal(data.socket);
}
// Close all sockets still in the poller
int rv = Poll.pollset(sendfilePollset, desc);
if (rv > 0) {
for (int n = 0; n < rv; n++) {
- closeSocket(desc[n*2+1]);
+ closeSocketInternal(desc[n*2+1]);
}
}
Pool.destroy(pool);
@@ -1824,7 +1802,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
Integer.valueOf(rv),
Error.strerror(rv)));
// Can't do anything: close the socket right away
- closeSocket(data.socket);
+ closeSocketInternal(data.socket);
}
}
addS.clear();
@@ -1846,7 +1824,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
remove(state);
// Destroy file descriptor pool, which should close the file
// Close the socket, as the response would be incomplete
- closeSocket(state.socket);
+ closeSocketInternal(state.socket);
continue;
}
// Write some data using sendfile
@@ -1858,7 +1836,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
remove(state);
// Close the socket, as the response would be incomplete
// This will close the file too.
- closeSocket(state.socket);
+ closeSocketInternal(state.socket);
continue;
}
@@ -1870,7 +1848,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
case NONE: {
// Close the socket since this is
// the end of the not keep-alive request.
- closeSocket(state.socket);
+ closeSocketInternal(state.socket);
break;
}
case PIPELINED: {
@@ -1879,7 +1857,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
Socket.timeoutSet(state.socket, getConnectionTimeout() * 1000);
// Process the pipelined request data
if (!processSocket(state.socket, SocketEvent.OPEN_READ)) {
- closeSocket(state.socket);
+ closeSocketInternal(state.socket);
}
break;
}
@@ -1928,7 +1906,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
remove(state);
// Destroy file descriptor pool, which should close the file
// Close the socket, as the response would be incomplete
- closeSocket(state.socket);
+ closeSocketInternal(state.socket);
}
}
}
@@ -1976,7 +1954,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
} else {
// Close socket and pool
getHandler().process(socket, SocketEvent.CONNECT_FAIL);
- closeSocket(socket.getSocket().longValue());
+ socket.close();
socket = null;
}
} else {
@@ -1984,7 +1962,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
if (!setSocketOptions(socket)) {
// Close socket and pool
getHandler().process(socket, SocketEvent.CONNECT_FAIL);
- closeSocket(socket.getSocket().longValue());
+ socket.close();
socket = null;
return;
}
@@ -1992,12 +1970,13 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
Handler.SocketState state = getHandler().process(socket, SocketEvent.OPEN_READ);
if (state == Handler.SocketState.CLOSED) {
// Close socket and pool
- closeSocket(socket.getSocket().longValue());
+ socket.close();
socket = null;
}
}
}
}
+
}
@@ -2021,7 +2000,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
SocketState state = getHandler().process(socketWrapper, event);
if (state == Handler.SocketState.CLOSED) {
// Close socket and pool
- closeSocket(socketWrapper.getSocket().longValue());
+ socketWrapper.close();
}
} finally {
socketWrapper = null;
@@ -2240,6 +2219,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
if (log.isDebugEnabled()) {
log.debug("Calling [" + getEndpoint() + "].closeSocket([" + this + "])");
}
+ getEndpoint().connections.remove(getSocket());
socketBufferHandler = SocketBufferHandler.EMPTY;
nonBlockingWriteBuffer.clear();
synchronized (closed) {
diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
index 5879fa9..ff5303b 100644
--- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
+++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
@@ -302,10 +302,10 @@ public class Nio2Endpoint extends AbstractJsseEndpoint<Nio2Channel,AsynchronousS
*/
@Override
protected boolean setSocketOptions(AsynchronousSocketChannel socket) {
- Nio2Channel channel = null;
- boolean success = false;
+ Nio2SocketWrapper socketWrapper = null;
try {
- socketProperties.setProperties(socket);
+ // Allocate channel and wrapper
+ Nio2Channel channel = null;
if (nioChannels != null) {
channel = nioChannels.pop();
}
@@ -320,31 +320,34 @@ public class Nio2Endpoint extends AbstractJsseEndpoint<Nio2Channel,AsynchronousS
channel = new Nio2Channel(bufhandler);
}
}
- Nio2SocketWrapper socketWrapper = new Nio2SocketWrapper(channel, this);
- connections.put(channel, socketWrapper);
- channel.reset(socket, socketWrapper);
+ Nio2SocketWrapper newWrapper = new Nio2SocketWrapper(channel, this);
+ channel.reset(socket, newWrapper);
+ connections.put(socket, newWrapper);
+ socketWrapper = newWrapper;
+
+ // Set socket properties
+ socketProperties.setProperties(socket);
+
socketWrapper.setReadTimeout(getConnectionTimeout());
socketWrapper.setWriteTimeout(getConnectionTimeout());
socketWrapper.setKeepAliveLeft(Nio2Endpoint.this.getMaxKeepAliveRequests());
socketWrapper.setSecure(isSSLEnabled());
// Continue processing on the same thread as the acceptor is async
- success = processSocket(socketWrapper, SocketEvent.OPEN_READ, false);
+ return processSocket(socketWrapper, SocketEvent.OPEN_READ, false);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
log.error(sm.getString("endpoint.socketOptionsError"), t);
- } finally {
- if (!success && channel != null) {
- connections.remove(channel);
- channel.free();
+ if (socketWrapper == null) {
+ destroySocket(socket);
}
}
// Tell to close the socket if needed
- return success;
+ return false;
}
@Override
- protected void closeSocket(AsynchronousSocketChannel socket) {
+ protected void destroySocket(AsynchronousSocketChannel socket) {
countDownConnection();
try {
socket.close();
@@ -922,6 +925,7 @@ public class Nio2Endpoint extends AbstractJsseEndpoint<Nio2Channel,AsynchronousS
log.debug("Calling [" + getEndpoint() + "].closeSocket([" + this + "])");
}
try {
+ getEndpoint().connections.remove(getSocket().getIOChannel());
synchronized (getSocket()) {
if (getSocket().isOpen()) {
getSocket().close(true);
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java b/java/org/apache/tomcat/util/net/NioEndpoint.java
index 090b26a..7501647 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -22,7 +22,6 @@ import java.io.FileInputStream;
import java.io.IOException;
import java.net.InetAddress;
import java.net.InetSocketAddress;
-import java.net.Socket;
import java.net.SocketTimeoutException;
import java.nio.ByteBuffer;
import java.nio.channels.CancelledKeyException;
@@ -393,15 +392,10 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
*/
@Override
protected boolean setSocketOptions(SocketChannel socket) {
- NioChannel channel = null;
- boolean success = false;
- // Process the connection
+ NioSocketWrapper socketWrapper = null;
try {
- // Disable blocking, polling will be used
- socket.configureBlocking(false);
- Socket sock = socket.socket();
- socketProperties.setProperties(sock);
-
+ // Allocate channel and wrapper
+ NioChannel channel = null;
if (nioChannels != null) {
channel = nioChannels.pop();
}
@@ -416,15 +410,22 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
channel = new NioChannel(bufhandler);
}
}
- NioSocketWrapper socketWrapper = new NioSocketWrapper(channel, this);
- connections.put(channel, socketWrapper);
- channel.reset(socket, socketWrapper);
+ NioSocketWrapper newWrapper = new NioSocketWrapper(channel, this);
+ channel.reset(socket, newWrapper);
+ connections.put(socket, newWrapper);
+ socketWrapper = newWrapper;
+
+ // Set socket properties
+ // Disable blocking, polling will be used
+ socket.configureBlocking(false);
+ socketProperties.setProperties(socket.socket());
+
socketWrapper.setReadTimeout(getConnectionTimeout());
socketWrapper.setWriteTimeout(getConnectionTimeout());
socketWrapper.setKeepAliveLeft(NioEndpoint.this.getMaxKeepAliveRequests());
socketWrapper.setSecure(isSSLEnabled());
poller.register(channel, socketWrapper);
- success = true;
+ return true;
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
try {
@@ -432,19 +433,17 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
} catch (Throwable tt) {
ExceptionUtils.handleThrowable(tt);
}
- } finally {
- if (!success && channel != null) {
- connections.remove(channel);
- channel.free();
+ if (socketWrapper == null) {
+ destroySocket(socket);
}
}
// Tell to close the socket if needed
- return success;
+ return false;
}
@Override
- protected void closeSocket(SocketChannel socket) {
+ protected void destroySocket(SocketChannel socket) {
countDownConnection();
try {
socket.close();
@@ -1170,6 +1169,7 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
log.debug("Calling [" + getEndpoint() + "].closeSocket([" + this + "])");
}
try {
+ getEndpoint().connections.remove(getSocket().getIOChannel());
synchronized (getSocket()) {
if (getSocket().isOpen()) {
getSocket().close(true);
diff --git a/java/org/apache/tomcat/util/net/SocketWrapperBase.java b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
index 169fbfa..1b700cb 100644
--- a/java/org/apache/tomcat/util/net/SocketWrapperBase.java
+++ b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
@@ -409,7 +409,6 @@ public abstract class SocketWrapperBase<E> {
log.error(sm.getString("endpoint.debug.handlerRelease"), e);
}
} finally {
- getEndpoint().connections.remove(socket);
getEndpoint().countDownConnection();
doClose();
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 3f70beb..0dab42e 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -45,6 +45,14 @@
issues do not "pop up" wrt. others).
-->
<section name="Tomcat 9.0.30 (markt)" rtext="in development">
+ <subsection name="Coyote">
+ <changelog>
+ <fix>
+ Fix endpoint closeSocket and destroySocket discrepancies, in particular
+ in the APR connector. (remm)
+ </fix>
+ </changelog>
+ </subsection>
<subsection name="Web applications">
<changelog>
<fix>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org