Bug 51467 - usage of method run instead of start to start a thread
Summary: usage of method run instead of start to start a thread
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: trunk
Hardware: All All
: P2 minor (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-03 10:19 UTC by Felix Schumacher
Modified: 2011-07-04 18:50 UTC (History)
0 users



Attachments
use start instead of run to start a thread (603 bytes, patch)
2011-07-03 10:20 UTC, Felix Schumacher
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Felix Schumacher 2011-07-03 10:19:46 UTC
In StandardContext method Thread#run is used. This is most probably a mistake, since a few lines below Thread#join is called to wait for the completion of the thread.

So we could either remove the join and get rid of the thread by using just a runnable, or start the thread.
Comment 1 Felix Schumacher 2011-07-03 10:20:20 UTC
Created attachment 27245 [details]
use start instead of run to start a thread
Comment 2 Christopher Schultz 2011-07-03 16:43:45 UTC
I'm wondering why the whole process is run in a Thread in the first place. If the thread is start()ed, when immediately join()ed, and all exceptions are documented not to be expected, it seems like the use of a thread is wasteful.

Your patch, while slightly more clear than the original code, does not appear to actually any behavior: the parent thread will still block, except this time the thread is actually launched.
Comment 3 Rainer Jung 2011-07-03 17:45:23 UTC
Re Comment #2: http://svn.apache.org/viewvc?view=revision&revision=1044145

Log says:

bug 49159: Improve ThreadLocal memory leak clean-up 
https://issues.apache.org/bugzilla/show_bug.cgi?id=49159

Use a dedicated thread when calling web application code when it is started and stopped (calls to Listeners, Filters, Servlets).

Code comment says:

// we do it in a dedicated thread for memory leak protection, in
// case some webapp code registers some ThreadLocals that they
// forget to cleanup

Regards,

Rainer
Comment 4 Christopher Schultz 2011-07-03 18:41:59 UTC
Rainer,

Aah, I didn't look far enough up in the code to see that comment: thanks for clarifying.

Without running Thread.start(), I believe any ThreadLocals registered will be registered to the calling thread, so Felix is right: changing Thread.run to Thread.start is appropriate in this case.
Comment 5 Mark Thomas 2011-07-04 12:50:46 UTC
Thanks for the patch.

Fixed in 7.0.x and will be included in 7.0.18 onwards.
Comment 6 Sylvain Laurent 2011-07-04 18:50:13 UTC
It seems there used to be call to DedicatedThreadExecutor.executeInOwnThread()
but r1087715 changed it to create a new Thread : http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/StandardContext.java?r1=1087715&r2=1087714&pathrev=1087715