Use of ThreadGroup in WildFly and WildFly Core
by Brian Stansberry
tl;dr; Should we be calling ThreadGroup.setMaxPriority in our ThreadGroups?
Any other thoughts on consistency in $subject?
Tomas Hofman has been doing some work cleaning up memory leaks that happen
when reload is called on a server, and one small one is ThreadGroup objects
leak. This is a small leak, as a ThreadGroup holds very little state if all
its threads have terminated. But it's good to clean things up, which has
led to a couple PRs:
https://github.com/wildfly/wildfly/pull/17192
https://github.com/wildfly/wildfly/pull/17193
Thinking about those got me thinking about how we use ThreadGroup in WF and
WF Core in general, and whether we should be more standard about it. Rather
than just discussing this on those PRs I figured I'd post here.
The first PR stores the TGs in constants. This works, but an alternative is
calling TG.setDaemon(true) on the thread group. Note that calling that
doesn't set a default for Thread.isDaemon() for the group's threads; all it
does is allow the TG to be terminated once all its threads terminate.
Which of these seems more appropriate depends on the intended lifecycle for
the threads; i.e. whether the expected thread count is at least one for the
lifetime of the group. Tomas' static field approach is more robust about
this. Calling setDaemon(true) on a TG stored in a constant is almost
certainly wrong.
A bit more interesting is Matej's approach in #17193. Here the TG is being
discarded. The effect of this is that the TG for the threads created by the
factory will be the TG of the thread that called the JBossThreadFactory
constructor. So, in this case, the TG of the MSC thread pool threads.
The basic downside of this approach is the code creating the
JBossThreadFactory has no idea what the settings of the current thread's TG
are and whether they are appropriate. It just assumes it's ok. However,
Matej's change doesn't actually change anything in this regard, since if
you create a new ThreadGroup and don't call setMaxPriority, your new TG
inherits the max priority from the current thread's TG.
It seems like if we create a new TG, we should call setMaxPriority on it,
unless we know what it has is ok.
Best regards,
Brian
2 years, 5 months
