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 <r...@apache.org> 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: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org