[GitHub] [tomcat] pzygielo commented on a change in pull request #275: BZ 59203 - interrupt tomcat threads instead of stopping

[GitBox <gi...@apache.org>]

pzygielo commented on a change in pull request #275: BZ 59203 - interrupt tomcat threads instead of stopping
URL: https://github.com/apache/tomcat/pull/275#discussion_r410846802
 
 

 ##########
 File path: java/org/apache/catalina/loader/WebappClassLoaderBase.java
 ##########
 @@ -1813,35 +1813,40 @@ private void clearReferencesThreads() {
                         // stopped and check them at the end of the method.
                         executorThreadsToStop.add(thread);
                     } else {
-                        thread.interrupt();
+                        clearThread(thread);
                     }
                 }
             }
         }
 
-        // If thread stopping is enabled, executor threads should have been
-        // stopped above when the executor was shut down but that depends on the
-        // thread correctly handling the interrupt. Give all the executor
-        // threads a few seconds shutdown and if they are still running
-        // Give threads up to 2 seconds to shutdown
-        int count = 0;
+        // clear executor threads
         for (Thread t : executorThreadsToStop) {
-            while (t.isAlive() && count < 100) {
-                try {
-                    Thread.sleep(20);
-                } catch (InterruptedException e) {
-                    // Quit the while loop
-                    break;
-                }
-                count++;
-            }
-            if (t.isAlive()) {
-                // This method is deprecated and for good reason. This is
-                // very risky code but is the only option at this point.
-                // A *very* good reason for apps to do this clean-up
-                // themselves.
-                t.stop();
-            }
+            clearThread(t);
+        }
+    }
+
+    private void clearThread(Thread t) {
+        int count = 0;
+        if (!t.isInterrupted()) {
+            t.interrupt();
+        }
+
+        // Give threads up to 2 seconds to shutdown
+        while (t.isAlive() && count < 100) {
+            try {
+                Thread.sleep(20);
+            } catch (InterruptedException e) {
+                // Quit the while loop
 
 Review comment:
   https://rules.sonarsource.com/java/RSPEC-2142

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org