You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2018/05/17 20:46:57 UTC
svn commit: r1831812 - in /tomcat/trunk:
java/org/apache/tomcat/util/net/AprEndpoint.java
java/org/apache/tomcat/util/net/LocalStrings.properties
res/findbugs/filter-false-positives.xml webapps/docs/changelog.xml
Author: markt
Date: Thu May 17 20:46:57 2018
New Revision: 1831812
URL: http://svn.apache.org/viewvc?rev=1831812&view=rev
Log:
Fix some SpotBugs warnings
While some warnings were false positives, some did point to some potential edge cases in the shutdown code. The code now explicitly checks that the thread has stopped rather than assuming it has.
Modified:
tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
tomcat/trunk/java/org/apache/tomcat/util/net/LocalStrings.properties
tomcat/trunk/res/findbugs/filter-false-positives.xml
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java?rev=1831812&r1=1831811&r2=1831812&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java Thu May 17 20:46:57 2018
@@ -682,20 +682,13 @@ public class AprEndpoint extends Abstrac
// Start poller thread
poller = new Poller();
poller.init();
- Thread pollerThread = new Thread(poller, getName() + "-Poller");
- pollerThread.setPriority(threadPriority);
- pollerThread.setDaemon(true);
- pollerThread.start();
+ poller.start();
// Start sendfile thread
if (getUseSendfile()) {
sendfile = new Sendfile();
sendfile.init();
- Thread sendfileThread =
- new Thread(sendfile, getName() + "-Sendfile");
- sendfileThread.setPriority(threadPriority);
- sendfileThread.setDaemon(true);
- sendfileThread.start();
+ sendfile.start();
}
startAcceptorThreads();
@@ -753,6 +746,7 @@ public class AprEndpoint extends Abstrac
connections.clear();
if (getUseSendfile()) {
try {
+ sendfile.stop();
sendfile.destroy();
} catch (Exception e) {
// Ignore
@@ -1278,7 +1272,7 @@ public class AprEndpoint extends Abstrac
private AtomicInteger connectionCount = new AtomicInteger(0);
public int getConnectionCount() { return connectionCount.get(); }
-
+ private volatile Thread pollerThread;
private volatile boolean pollerRunning = true;
/**
@@ -1348,12 +1342,22 @@ public class AprEndpoint extends Abstrac
}
+ protected void start() {
+ pollerThread = new Thread(poller, getName() + "-Poller");
+ pollerThread.setPriority(threadPriority);
+ pollerThread.setDaemon(true);
+ pollerThread.start();
+ }
+
+
/*
* This method is synchronized so that it is not possible for a socket
* to be added to the Poller's addList once this method has completed.
*/
protected synchronized void stop() {
pollerRunning = false;
+ // In case the poller thread is in the idle wait
+ this.notify();
}
@@ -1361,14 +1365,20 @@ public class AprEndpoint extends Abstrac
* Destroy the poller.
*/
protected synchronized void destroy() {
- // Wait for pollerTime before doing anything, so that the poller
- // threads exit, otherwise parallel destruction of sockets which are
- // still in the poller can cause problems
- try {
- this.notify();
- this.wait(pollerCount * pollTime / 1000);
- } catch (InterruptedException e) {
- // Ignore
+ // Wait for the poller thread to exit, otherwise parallel
+ // destruction of sockets which are still in the poller can cause
+ // problems.
+ int loops = pollerCount * 50;
+ while (loops > 0 && pollerThread.isAlive()) {
+ try {
+ this.wait(pollTime / 1000);
+ } catch (InterruptedException e) {
+ // Ignore
+ }
+ loops--;
+ }
+ if (pollerThread.isAlive()) {
+ log.warn(sm.getString("endpoint.pollerThreadStop"));
}
// Close all sockets in the close queue
SocketInfo info = closeList.get();
@@ -1441,6 +1451,7 @@ public class AprEndpoint extends Abstrac
// Add socket to the list. Newly added sockets will wait
// at most for pollTime before being polled.
if (addList.add(socket, timeout, flags)) {
+ // In case the poller thread is in the idle wait
this.notify();
}
}
@@ -1474,6 +1485,7 @@ public class AprEndpoint extends Abstrac
*/
private synchronized void close(long socket) {
closeList.add(socket, 0, 0);
+ // In case the poller thread is in the idle wait
this.notify();
}
@@ -1578,7 +1590,7 @@ public class AprEndpoint extends Abstrac
// with no processing since the notify() call in
// add()/close() would have no effect since it
// happened before this sync block was entered
- if (addList.size() < 1 && closeList.size() < 1) {
+ if (pollerRunning && addList.size() < 1 && closeList.size() < 1) {
this.wait(10000);
}
}
@@ -1921,6 +1933,7 @@ public class AprEndpoint extends Abstrac
protected ArrayList<SendfileData> addS;
+ private volatile Thread sendfileThread;
private volatile boolean sendfileRunning = true;
/**
@@ -1948,22 +1961,40 @@ public class AprEndpoint extends Abstrac
addS = new ArrayList<>();
}
- /**
- * Destroy the poller.
- */
- protected void destroy() {
+ protected void start() {
+ sendfileThread = new Thread(sendfile, getName() + "-Sendfile");
+ sendfileThread.setPriority(threadPriority);
+ sendfileThread.setDaemon(true);
+ sendfileThread.start();
+ }
+
+ protected void stop() {
sendfileRunning = false;
- // Wait for polltime before doing anything, so that the poller threads
- // exit, otherwise parallel destruction of sockets which are still
- // in the poller can cause problems
- try {
- synchronized (this) {
- this.notify();
+
+ // Wait for the sendfile thread to exit, otherwise parallel
+ // destruction of sockets which are still in the poller can cause
+ // problems.
+ int loops = 50;
+ while (loops > 0 && sendfileThread.isAlive()) {
+ try {
this.wait(pollTime / 1000);
+ } catch (InterruptedException e) {
+ // Ignore
}
- } catch (InterruptedException e) {
- // Ignore
+ loops--;
}
+ if (sendfileThread.isAlive()) {
+ log.warn(sm.getString("endpoint.sendfileThreadStop"));
+ }
+
+ // In case the sendfile thread is in the idle wait
+ this.notify();
+ }
+
+ /**
+ * Destroy the poller.
+ */
+ protected void destroy() {
// Close any socket remaining in the add queue
for (int i = (addS.size() - 1); i >= 0; i--) {
SendfileData data = addS.get(i);
Modified: tomcat/trunk/java/org/apache/tomcat/util/net/LocalStrings.properties
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/LocalStrings.properties?rev=1831812&r1=1831811&r2=1831812&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/LocalStrings.properties (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/LocalStrings.properties Thu May 17 20:46:57 2018
@@ -59,11 +59,13 @@ endpoint.poll.limitedpollsize=Failed to
endpoint.poll.initfail=Poller creation failed
endpoint.poll.fail=Critical poller failure (restarting poller): [{0}] [{1}]
endpoint.poll.error=Unexpected poller error
+endpoint.pollerThreadStop=The poller thread failed to stop in a timely manner
endpoint.process.fail=Error allocating socket processor
endpoint.processing.fail=Error running socket processor
endpoint.removeDefaultSslHostConfig=The default SSLHostConfig (named [{0}]) may not be removed
-endpoint.sendfile.error=Unexpected sendfile error
endpoint.sendfile.addfail=Sendfile failure: [{0}] [{1}]
+endpoint.sendfile.error=Unexpected sendfile error
+endpoint.sendfileThreadStop=The sendfile thread failed to stop in a timely manner
endpoint.setAttribute=Set [{0}] to [{1}]
endpoint.timeout.err=Error processing socket timeout
endpoint.unknownSslHostName=The SSL host name [{0}] is not recognised for this endpoint
Modified: tomcat/trunk/res/findbugs/filter-false-positives.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/res/findbugs/filter-false-positives.xml?rev=1831812&r1=1831811&r2=1831812&view=diff
==============================================================================
--- tomcat/trunk/res/findbugs/filter-false-positives.xml (original)
+++ tomcat/trunk/res/findbugs/filter-false-positives.xml Thu May 17 20:46:57 2018
@@ -1092,6 +1092,11 @@
<Bug pattern="UUF_UNUSED_PUBLIC_OR_PROTECTED_FIELD" />
</Match>
<Match>
+ <!-- Field is populated by JNI code -->
+ <Class name="org.apache.tomcat.jni.Sockaddr" />
+ <Bug pattern="UWF_UNWRITTEN_PUBLIC_OR_PROTECTED_FIELD"/>
+ </Match>
+ <Match>
<Class name="org.apache.tomcat.util.IntrospectionUtils" />
<Method name="findMethod"/>
<Bug code="NP" />
@@ -1245,21 +1250,22 @@
<Bug code="MF" />
</Match>
<Match>
- <!-- JSSE vs APR attribute names. More confusing to change one of them -->
- <Class name="org.apache.tomcat.util.net.AprEndpoint"/>
- <Or>
- <Method name="getSSLProtocol"/>
- <Method name="setSSLProtocol"/>
- </Or>
- <Bug code="Nm"/>
- </Match>
- <Match>
<!-- See wait() call in destroy() -->
<Class name="org.apache.tomcat.util.net.AprEndpoint$Poller"/>
<Method name="run"/>
<Bug code="NN" />
</Match>
<Match>
+ <!-- There is only a single wait in run() when the poller is idle -->
+ <Class name="org.apache.tomcat.util.net.AprEndpoint$Poller"/>
+ <Or>
+ <Method name="add"/>
+ <Method name="close"/>
+ <Method name="stop"/>
+ </Or>
+ <Bug pattern="NO_NOTIFY_NOT_NOTIFYALL" />
+ </Match>
+ <Match>
<Class name="org.apache.tomcat.util.net.AprEndpoint$Sendfile"/>
<Method name="run"/>
<Or>
@@ -1270,6 +1276,15 @@
</Or>
</Match>
<Match>
+ <!-- There is only a single wait in run() when the poller is idle -->
+ <Class name="org.apache.tomcat.util.net.AprEndpoint$Sendfile"/>
+ <Or>
+ <Method name="add"/>
+ <Method name="stop"/>
+ </Or>
+ <Bug pattern="NO_NOTIFY_NOT_NOTIFYALL" />
+ </Match>
+ <Match>
<!-- Sync is there to protect referenced object not field -->
<Class name="org.apache.tomcat.util.net.AprEndpoint$SocketEventProcessor"/>
<Method name="run"/>
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1831812&r1=1831811&r2=1831812&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Thu May 17 20:46:57 2018
@@ -123,6 +123,10 @@
<fix>
<bug>62371</bug>: Improve logging of Host validation failures. (markt)
</fix>
+ <fix>
+ Fix a couple of unlikely edge cases in the shutting down of the
+ APR/native connector. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org