You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2020/04/18 10:11:25 UTC
[GitHub] [tomcat] govi20 opened a new pull request #275: BZ 59203 -
interrupt tomcat threads instead of stopping
govi20 opened a new pull request #275: BZ 59203 - interrupt tomcat threads instead of stopping
URL: https://github.com/apache/tomcat/pull/275
interrupt tomcat threads instead of stopping. It seems existing test cases are already checks whether the ThreadPoolExecutor is terminated.
----------------------------------------------------------------
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
[GitHub] [tomcat] pzygielo commented on a change in pull request #275: BZ
59203 - interrupt tomcat threads instead of stopping
Posted by 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
[GitHub] [tomcat] govi20 commented on issue #275: BZ 59203 - interrupt
tomcat threads instead of stopping
Posted by GitBox <gi...@apache.org>.
govi20 commented on issue #275: BZ 59203 - interrupt tomcat threads instead of stopping
URL: https://github.com/apache/tomcat/pull/275#issuecomment-615931189
I will remove the cleanup changes.
So apparently we should call interrupt, check if thread is still active, if yes then call stop. Correct me if I'm wrong here.
----------------------------------------------------------------
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
[GitHub] [tomcat] markt-asf commented on issue #275: BZ 59203 - interrupt
tomcat threads instead of stopping
Posted by GitBox <gi...@apache.org>.
markt-asf commented on issue #275: BZ 59203 - interrupt tomcat threads instead of stopping
URL: https://github.com/apache/tomcat/pull/275#issuecomment-615904750
Thanks for the PR.
There are multiple different changes in this commit. It needs to be broken down into one commit for the changed described in the PR summary and one commit for each type of clean-up.
The configuration option that controls this behaviour is called `clearReferencesStopThreads`. That makes no sense with this PR applied. However, the behaviour of stopping threads needs to be retained.
Note there are multiple places where stop() is called. This PR addresses only one of them.
It would be better to align the behaviour for Executor and non-Executor threads. i.e.:
- interrupt
- pause
- stop if still running
There was some debate whether there should be multiple iterations if interrupt/pause but I'm leaning towards not. We are dealing with broken web applications here.
----------------------------------------------------------------
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