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 c6cf747 More close fixes
c6cf747 is described below
commit c6cf74737c62399010f9abead54b8ec5a6f104a3
Author: remm <[email protected]>
AuthorDate: Thu May 16 11:28:36 2019 +0200
More close fixes
Fix a NIO2 problem where sockets were discarded on close, now it
processes a STOP instead (NIO does that in one case). Also use the
return value of processSocket when it is using the executor. Three
sockets are not getting closed in
catalina.authenticator.TestSSOnonLoginAndBasicAuthenticator for some
unknown reason (a read is pending, but the completion handler is never
called on shutdown - it should get a AsynchronousCloseException).
Fix the 3 unclosed sockets for NIO, apparently caused by using the key
attachment being null. Redo cancelledKey without it and simplify.
---
java/org/apache/tomcat/util/net/AprEndpoint.java | 33 +++++----
java/org/apache/tomcat/util/net/Nio2Endpoint.java | 38 +++++++----
java/org/apache/tomcat/util/net/NioEndpoint.java | 82 ++++++++++-------------
webapps/docs/changelog.xml | 3 +
4 files changed, 81 insertions(+), 75 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java
b/java/org/apache/tomcat/util/net/AprEndpoint.java
index 9551b7e..984452e 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -674,30 +674,26 @@ public class AprEndpoint extends
AbstractEndpoint<Long,Long> implements SNICallB
@Override
protected boolean setSocketOptions(Long socket) {
try {
- // During shutdown, executor may be null - avoid NPE
- if (running) {
- if (log.isDebugEnabled()) {
- log.debug(sm.getString("endpoint.debug.socket", socket));
- }
- AprSocketWrapper wrapper = new AprSocketWrapper(socket, this);
- wrapper.setKeepAliveLeft(getMaxKeepAliveRequests());
- wrapper.setSecure(isSSLEnabled());
- wrapper.setReadTimeout(getConnectionTimeout());
- wrapper.setWriteTimeout(getConnectionTimeout());
- connections.put(socket, wrapper);
- getExecutor().execute(new SocketWithOptionsProcessor(wrapper));
- }
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("endpoint.debug.socket", socket));
+ }
+ AprSocketWrapper wrapper = new AprSocketWrapper(socket, this);
+ wrapper.setKeepAliveLeft(getMaxKeepAliveRequests());
+ wrapper.setSecure(isSSLEnabled());
+ wrapper.setReadTimeout(getConnectionTimeout());
+ wrapper.setWriteTimeout(getConnectionTimeout());
+ connections.put(socket, wrapper);
+ getExecutor().execute(new SocketWithOptionsProcessor(wrapper));
+ return true;
} catch (RejectedExecutionException x) {
log.warn(sm.getString("endpoint.rejectedExecution", socket), x);
- return false;
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
// This means we got an OOM or similar creating a thread, or that
// the pool and its queue are full
log.error(sm.getString("endpoint.process.fail"), t);
- return false;
}
- return true;
+ return false;
}
@@ -2337,6 +2333,9 @@ public class AprEndpoint extends
AbstractEndpoint<Long,Long> implements SNICallB
@Override
protected void doClose() {
+ if (log.isDebugEnabled()) {
+ log.debug("Calling [" + getEndpoint() + "].closeSocket([" +
this + "])", new Exception());
+ }
try {
getEndpoint().getHandler().release(this);
} catch (Throwable e) {
@@ -2760,7 +2759,7 @@ public class AprEndpoint extends
AbstractEndpoint<Long,Long> implements SNICallB
timeout, unit, attachment, check, handler, semaphore,
completion);
}
- private class AprOperationState<A> extends OperationState<A> {
+ private class AprOperationState<A> extends OperationState<A> {
private volatile boolean inline = true;
private AprOperationState(boolean read, ByteBuffer[] buffers, int
offset, int length,
BlockingMode block, long timeout, TimeUnit unit, A
attachment, CompletionCheck check,
diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
index 613f057..c5a3328 100644
--- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
+++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
@@ -507,8 +507,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
break;
}
case PIPELINED: {
-
getEndpoint().processSocket(Nio2SocketWrapper.this,
- SocketEvent.OPEN_READ, true);
+ if
(!getEndpoint().processSocket(Nio2SocketWrapper.this, SocketEvent.OPEN_READ,
true)) {
+ close();
+ }
break;
}
case OPEN: {
@@ -602,9 +603,10 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
// notify/dispatch to do the release.
readPending.release();
// If already closed, don't call onError and close
again
- return;
+ getEndpoint().processSocket(Nio2SocketWrapper.this,
SocketEvent.STOP, false);
+ } else if
(!getEndpoint().processSocket(Nio2SocketWrapper.this, SocketEvent.ERROR, true))
{
+ close();
}
- getEndpoint().processSocket(Nio2SocketWrapper.this,
SocketEvent.ERROR, true);
}
};
@@ -641,7 +643,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
}
}
if (notify) {
- endpoint.processSocket(Nio2SocketWrapper.this,
SocketEvent.OPEN_WRITE, true);
+ if (!endpoint.processSocket(Nio2SocketWrapper.this,
SocketEvent.OPEN_WRITE, true)) {
+ close();
+ }
}
}
@Override
@@ -654,7 +658,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
}
setError(ioe);
writePending.release();
- endpoint.processSocket(Nio2SocketWrapper.this,
SocketEvent.ERROR, true);
+ if (!endpoint.processSocket(Nio2SocketWrapper.this,
SocketEvent.ERROR, true)) {
+ close();
+ }
}
};
@@ -687,7 +693,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
}
}
if (notify) {
- endpoint.processSocket(Nio2SocketWrapper.this,
SocketEvent.OPEN_WRITE, true);
+ if (!endpoint.processSocket(Nio2SocketWrapper.this,
SocketEvent.OPEN_WRITE, true)) {
+ close();
+ }
}
}
@Override
@@ -700,7 +708,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
}
setError(ioe);
writePending.release();
- endpoint.processSocket(Nio2SocketWrapper.this,
SocketEvent.ERROR, true);
+ if (!endpoint.processSocket(Nio2SocketWrapper.this,
SocketEvent.ERROR, true)) {
+ close();
+ }
}
};
@@ -918,8 +928,6 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
if (getSocket().isOpen()) {
getSocket().close(true);
}
- socketBufferHandler = SocketBufferHandler.EMPTY;
- nonBlockingWriteBuffer.clear();
if (getEndpoint().running && !getEndpoint().paused) {
if (nioChannels == null ||
!nioChannels.push(getSocket())) {
getSocket().free();
@@ -932,6 +940,8 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
log.error(sm.getString("endpoint.debug.channelCloseFail"),
e);
}
} finally {
+ socketBufferHandler = SocketBufferHandler.EMPTY;
+ nonBlockingWriteBuffer.clear();
reset(Nio2Channel.CLOSED_NIO2_CHANNEL);
}
try {
@@ -1363,7 +1373,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
if (fillReadBuffer(false) > 0) {
// Special case where the read completed inline,
there is no notification
// in that case so it has to be done here
- getEndpoint().processSocket(this,
SocketEvent.OPEN_READ, true);
+ if (!getEndpoint().processSocket(this,
SocketEvent.OPEN_READ, true)) {
+ close();
+ }
}
} catch (IOException e) {
// Will never happen
@@ -1384,7 +1396,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
writeInterest = true;
if (writePending.availablePermits() == 1) {
// If no write is pending, notify that writing is possible
- getEndpoint().processSocket(this, SocketEvent.OPEN_WRITE,
true);
+ if (!getEndpoint().processSocket(this,
SocketEvent.OPEN_WRITE, true)) {
+ close();
+ }
}
}
}
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java
b/java/org/apache/tomcat/util/net/NioEndpoint.java
index 8f87f06..3677059 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -424,6 +424,7 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
socketWrapper.setKeepAliveLeft(NioEndpoint.this.getMaxKeepAliveRequests());
socketWrapper.setSecure(isSSLEnabled());
poller.register(channel, socketWrapper);
+ return true;
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
try {
@@ -431,10 +432,9 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
} catch (Throwable tt) {
ExceptionUtils.handleThrowable(tt);
}
- // Tell to close the socket
- return false;
}
- return true;
+ // Tell to close the socket
+ return false;
}
@@ -534,12 +534,12 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
socketWrapper.interestOps(ops);
key.interestOps(ops);
} else {
-
socket.getSocketWrapper().getPoller().cancelledKey(key);
+
socket.getSocketWrapper().getPoller().cancelledKey(key,
socket.getSocketWrapper());
}
}
} catch (CancelledKeyException ckx) {
try {
-
socket.getSocketWrapper().getPoller().cancelledKey(key);
+
socket.getSocketWrapper().getPoller().cancelledKey(key,
socket.getSocketWrapper());
} catch (Exception ignore) {}
}
}
@@ -667,24 +667,21 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
addEvent(r);
}
- public NioSocketWrapper cancelledKey(SelectionKey sk) {
- NioSocketWrapper socketWrapper = null;
+ public void cancelledKey(SelectionKey sk,
SocketWrapperBase<NioChannel> socketWrapper) {
try {
- if (sk == null) {
- // Nothing to do
- return null;
- }
- socketWrapper = (NioSocketWrapper) sk.attach(null);
if (socketWrapper != null) {
socketWrapper.close();
}
- if (sk.isValid()) {
- sk.cancel();
- }
- // The SocketChannel is also available via the SelectionKey. If
- // it hasn't been closed in the block above, close it now.
- if (sk.channel().isOpen()) {
- sk.channel().close();
+ if (sk != null) {
+ sk.attach(null);
+ if (sk.isValid()) {
+ sk.cancel();
+ }
+ // The SocketChannel is also available via the
SelectionKey. If
+ // it hasn't been closed in the block above, close it now.
+ if (sk.channel().isOpen()) {
+ sk.channel().close();
+ }
}
} catch (Throwable e) {
ExceptionUtils.handleThrowable(e);
@@ -692,7 +689,6 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
log.error(sm.getString("endpoint.debug.channelCloseFail"),
e);
}
}
- return socketWrapper;
}
/**
@@ -766,7 +762,7 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
protected void processKey(SelectionKey sk, NioSocketWrapper
socketWrapper) {
try {
if (close) {
- cancelledKey(sk);
+ cancelledKey(sk, socketWrapper);
} else if (sk.isValid() && socketWrapper != null) {
if (sk.isReadable() || sk.isWritable()) {
if (socketWrapper.getSendfileData() != null) {
@@ -794,16 +790,16 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
}
}
if (closeSocket) {
- cancelledKey(sk);
+ cancelledKey(sk, socketWrapper);
}
}
}
} else {
// Invalid key
- cancelledKey(sk);
+ cancelledKey(sk, socketWrapper);
}
} catch (CancelledKeyException ckx) {
- cancelledKey(sk);
+ cancelledKey(sk, socketWrapper);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
log.error(sm.getString("endpoint.nio.keyProcessingError"), t);
@@ -871,8 +867,7 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
if (log.isDebugEnabled()) {
log.debug("Send file connection is being
closed");
}
- poller.cancelledKey(sk);
- socketWrapper.close();
+ poller.cancelledKey(sk, socketWrapper);
break;
}
case PIPELINED: {
@@ -880,8 +875,7 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
log.debug("Connection is keep alive,
processing pipe-lined data");
}
if (!processSocket(socketWrapper,
SocketEvent.OPEN_READ, true)) {
- poller.cancelledKey(sk);
- socketWrapper.close();
+ poller.cancelledKey(sk, socketWrapper);
}
break;
}
@@ -911,15 +905,13 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
log.debug("Unable to complete sendfile request:", e);
}
if (!calledByProcessor && sc != null) {
- poller.cancelledKey(sk);
- socketWrapper.close();
+ poller.cancelledKey(sk, socketWrapper);
}
return SendfileState.ERROR;
} catch (Throwable t) {
log.error(sm.getString("endpoint.sendfile.error"), t);
if (!calledByProcessor && sc != null) {
- poller.cancelledKey(sk);
- socketWrapper.close();
+ poller.cancelledKey(sk, socketWrapper);
}
return SendfileState.ERROR;
}
@@ -955,12 +947,12 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
NioSocketWrapper socketWrapper = (NioSocketWrapper)
key.attachment();
if (socketWrapper == null) {
// We don't support any keys without attachments
- cancelledKey(key);
+ cancelledKey(key, null);
} else if (close) {
key.interestOps(0);
// Avoid duplicate stop calls
socketWrapper.interestOps(0);
- processKey(key,socketWrapper);
+ cancelledKey(key, socketWrapper);
} else if ((socketWrapper.interestOps() &
SelectionKey.OP_READ) == SelectionKey.OP_READ ||
(socketWrapper.interestOps() &
SelectionKey.OP_WRITE) == SelectionKey.OP_WRITE) {
boolean isTimedOut = false;
@@ -987,19 +979,19 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
socketWrapper.setError(new
SocketTimeoutException());
if (readTimeout && socketWrapper.readOperation
!= null) {
if
(!socketWrapper.readOperation.process()) {
- cancelledKey(key);
+ cancelledKey(key, socketWrapper);
}
} else if (writeTimeout &&
socketWrapper.writeOperation != null) {
if
(!socketWrapper.writeOperation.process()) {
- cancelledKey(key);
+ cancelledKey(key, socketWrapper);
}
} else if (!processSocket(socketWrapper,
SocketEvent.ERROR, true)) {
- cancelledKey(key);
+ cancelledKey(key, socketWrapper);
}
}
}
} catch (CancelledKeyException ckx) {
- cancelledKey(key);
+ cancelledKey(key, (NioSocketWrapper) key.attachment());
}
}
} catch (ConcurrentModificationException cme) {
@@ -1192,8 +1184,6 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
if (getSocket().isOpen()) {
getSocket().close(true);
}
- socketBufferHandler = SocketBufferHandler.EMPTY;
- nonBlockingWriteBuffer.clear();
if (getEndpoint().running && !getEndpoint().paused) {
if (nioChannels == null ||
!nioChannels.push(getSocket())) {
getSocket().free();
@@ -1206,6 +1196,8 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
log.error(sm.getString("endpoint.debug.channelCloseFail"),
e);
}
} finally {
+ socketBufferHandler = SocketBufferHandler.EMPTY;
+ nonBlockingWriteBuffer.clear();
reset(NioChannel.CLOSED_NIO_CHANNEL);
}
try {
@@ -1576,24 +1568,22 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
state = getHandler().process(socketWrapper, event);
}
if (state == SocketState.CLOSED) {
- poller.cancelledKey(key);
- socketWrapper.close();
+ poller.cancelledKey(key, socketWrapper);
}
} else if (handshake == -1 ) {
- poller.cancelledKey(key);
- socketWrapper.close();
+ poller.cancelledKey(key, socketWrapper);
} else if (handshake == SelectionKey.OP_READ){
socketWrapper.registerReadInterest();
} else if (handshake == SelectionKey.OP_WRITE){
socketWrapper.registerWriteInterest();
}
} catch (CancelledKeyException cx) {
- poller.cancelledKey(key);
+ poller.cancelledKey(key, socketWrapper);
} catch (VirtualMachineError vme) {
ExceptionUtils.handleThrowable(vme);
} catch (Throwable t) {
log.error(sm.getString("endpoint.processing.fail"), t);
- poller.cancelledKey(key);
+ poller.cancelledKey(key, socketWrapper);
} finally {
socketWrapper = null;
event = null;
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 0a0d1e3..b548b7c 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -124,6 +124,9 @@
<fix>
Clear buffers on socket wrapper close. (remm)
</fix>
+ <fix>
+ NIO2 failed to properly close sockets on connector stop. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Other">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
