This is an automated email from the ASF dual-hosted git repository.
markt 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 7343eed Refactor the stopping of the acceptor.
7343eed is described below
commit 7343eeddcc310af85a677613a4dd58a1e30984c6
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Aug 13 17:10:48 2020 +0100
Refactor the stopping of the acceptor.
Can't use !endpoint.isRunning() to trigger the stopping of the acceptor
thread as endpoint.stop() followed immediately by endpoint.start() can
result in the acceptor thread never seeing endpoint.isRunning()
returning false.
---
java/org/apache/tomcat/util/net/Acceptor.java | 156 +++++++++++++---------
java/org/apache/tomcat/util/net/AprEndpoint.java | 17 +--
java/org/apache/tomcat/util/net/Nio2Endpoint.java | 7 +-
java/org/apache/tomcat/util/net/NioEndpoint.java | 1 +
webapps/docs/changelog.xml | 5 +
5 files changed, 105 insertions(+), 81 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/Acceptor.java
b/java/org/apache/tomcat/util/net/Acceptor.java
index c09bf4b..9fa0818 100644
--- a/java/org/apache/tomcat/util/net/Acceptor.java
+++ b/java/org/apache/tomcat/util/net/Acceptor.java
@@ -16,6 +16,9 @@
*/
package org.apache.tomcat.util.net;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.jni.Error;
@@ -32,6 +35,13 @@ public class Acceptor<U> implements Runnable {
private final AbstractEndpoint<?,U> endpoint;
private String threadName;
+ /*
+ * Tracked separately rather than using endpoint.isRunning() as calls to
+ * endpoint.stop() and endpoint.start() in quick succession can cause the
+ * acceptor to continue running when it should terminate.
+ */
+ private volatile boolean stopCalled = false;
+ private final CountDownLatch stopLatch = new CountDownLatch(1);
protected volatile AcceptorState state = AcceptorState.NEW;
@@ -60,88 +70,102 @@ public class Acceptor<U> implements Runnable {
int errorDelay = 0;
- // Loop until we receive a shutdown command
- while (endpoint.isRunning()) {
-
- // Loop if endpoint is paused
- while (endpoint.isPaused() && endpoint.isRunning()) {
- state = AcceptorState.PAUSED;
- try {
- Thread.sleep(50);
- } catch (InterruptedException e) {
- // Ignore
+ try {
+ // Loop until we receive a shutdown command
+ while (!stopCalled) {
+
+ // Loop if endpoint is paused
+ while (endpoint.isPaused() && !stopCalled) {
+ state = AcceptorState.PAUSED;
+ try {
+ Thread.sleep(50);
+ } catch (InterruptedException e) {
+ // Ignore
+ }
}
- }
- if (!endpoint.isRunning()) {
- break;
- }
- state = AcceptorState.RUNNING;
-
- try {
- //if we have reached max connections, wait
- endpoint.countUpOrAwaitConnection();
-
- // Endpoint might have been paused while waiting for latch
- // If that is the case, don't accept new connections
- if (endpoint.isPaused()) {
- continue;
+ if (stopCalled) {
+ break;
}
+ state = AcceptorState.RUNNING;
- U socket = null;
try {
- // Accept the next incoming connection from the server
- // socket
- socket = endpoint.serverSocketAccept();
- } catch (Exception ioe) {
- // We didn't get a socket
- endpoint.countDownConnection();
- if (endpoint.isRunning()) {
- // Introduce delay if necessary
- errorDelay = handleExceptionWithDelay(errorDelay);
- // re-throw
- throw ioe;
- } else {
- break;
+ //if we have reached max connections, wait
+ endpoint.countUpOrAwaitConnection();
+
+ // Endpoint might have been paused while waiting for latch
+ // If that is the case, don't accept new connections
+ if (endpoint.isPaused()) {
+ continue;
}
- }
- // Successful accept, reset the error delay
- errorDelay = 0;
-
- // Configure the socket
- if (endpoint.isRunning() && !endpoint.isPaused()) {
- // setSocketOptions() will hand the socket off to
- // an appropriate processor if successful
- if (!endpoint.setSocketOptions(socket)) {
- endpoint.closeSocket(socket);
+
+ U socket = null;
+ try {
+ // Accept the next incoming connection from the server
+ // socket
+ socket = endpoint.serverSocketAccept();
+ } catch (Exception ioe) {
+ // We didn't get a socket
+ endpoint.countDownConnection();
+ if (endpoint.isRunning()) {
+ // Introduce delay if necessary
+ errorDelay = handleExceptionWithDelay(errorDelay);
+ // re-throw
+ throw ioe;
+ } else {
+ break;
+ }
}
- } else {
- endpoint.destroySocket(socket);
- }
- } catch (Throwable t) {
- ExceptionUtils.handleThrowable(t);
- String msg = sm.getString("endpoint.accept.fail");
- // APR specific.
- // Could push this down but not sure it is worth the trouble.
- if (t instanceof Error) {
- Error e = (Error) t;
- if (e.getError() == 233) {
- // Not an error on HP-UX so log as a warning
- // so it can be filtered out on that platform
- // See bug 50273
- log.warn(msg, t);
+ // Successful accept, reset the error delay
+ errorDelay = 0;
+
+ // Configure the socket
+ if (!stopCalled && !endpoint.isPaused()) {
+ // setSocketOptions() will hand the socket off to
+ // an appropriate processor if successful
+ if (!endpoint.setSocketOptions(socket)) {
+ endpoint.closeSocket(socket);
+ }
+ } else {
+ endpoint.destroySocket(socket);
+ }
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+ String msg = sm.getString("endpoint.accept.fail");
+ // APR specific.
+ // Could push this down but not sure it is worth the
trouble.
+ if (t instanceof Error) {
+ Error e = (Error) t;
+ if (e.getError() == 233) {
+ // Not an error on HP-UX so log as a warning
+ // so it can be filtered out on that platform
+ // See bug 50273
+ log.warn(msg, t);
+ } else {
+ log.error(msg, t);
+ }
} else {
- log.error(msg, t);
+ log.error(msg, t);
}
- } else {
- log.error(msg, t);
}
}
+ } finally {
+ stopLatch.countDown();
}
state = AcceptorState.ENDED;
}
+ public void stop() {
+ stopCalled = true;
+ try {
+ stopLatch.await(10, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ // Ignore
+ }
+ }
+
+
/**
* Handles exceptions where a delay is required to prevent a Thread from
* entering a tight loop which will consume CPU and may also trigger large
diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java
b/java/org/apache/tomcat/util/net/AprEndpoint.java
index 8f1a7eb..57e6d5f 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -486,24 +486,13 @@ public class AprEndpoint extends
AbstractEndpoint<Long,Long> implements SNICallB
}
if (running) {
running = false;
+ acceptor.stop();
poller.stop();
for (SocketWrapperBase<Long> socketWrapper : connections.values())
{
socketWrapper.close();
}
- long waitLeft = 10000;
- while (waitLeft > 0 &&
- acceptor.getState() != AcceptorState.ENDED &&
- serverSock != 0) {
- try {
- Thread.sleep(50);
- } catch (InterruptedException e) {
- // Ignore
- }
- waitLeft -= 50;
- }
- if (waitLeft == 0) {
- log.warn(sm.getString("endpoint.warn.unlockAcceptorFailed",
- acceptor.getThreadName()));
+ if (acceptor.getState() != AcceptorState.ENDED) {
+ log.warn(sm.getString("endpoint.warn.unlockAcceptorFailed",
acceptor.getThreadName()));
// If the Acceptor is still running force
// the hard socket close.
if (serverSock != 0) {
diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
index 2e3a5af..1d218a5 100644
--- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
+++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
@@ -192,7 +192,7 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
}
if (running) {
running = false;
- acceptor.state = AcceptorState.ENDED;
+ acceptor.stop();
// Use the executor to avoid binding the main thread if something
bad
// occurs and unbind will also wait for a bit for it to complete
getExecutor().execute(new Runnable() {
@@ -408,6 +408,11 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
}
@Override
+ public void stop() {
+ acceptor.state = AcceptorState.ENDED;
+ }
+
+ @Override
public void completed(AsynchronousSocketChannel socket,
Void attachment) {
// Successful accept, reset the error delay
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java
b/java/org/apache/tomcat/util/net/NioEndpoint.java
index 9bff874..527f38d 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -238,6 +238,7 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
}
if (running) {
running = false;
+ acceptor.stop();
if (poller != null) {
poller.destroy();
poller = null;
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 7f012a4..52ca572 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -99,6 +99,11 @@
Avoid several potential NPEs introduced in the changes in the previous
release to reduce the memory footprint of closed HTTP/2 streams.
(markt)
</fix>
+ <fix>
+ Refactor the stopping of the acceptor to ensure that the acceptor
thread
+ stops when a connector is started immediately after it is stopped.
+ (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="WebSocket">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
