WFCORE-6505 Avoid creating duplicate thread groups on server reload #5672
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
deps-ok
TomasHofman
commented
Sep 20, 2023
Comment on lines
142
to
148
| final ThreadFactoryService service = new ThreadFactoryService(); | ||
| service.setThreadGroupName(getThreadGroupName(threadPoolName)); | ||
| service.setNamePattern("%G - %t"); | ||
|
|
||
| // Do not set service#threadGroupName, make the group name part of the pattern instead. Setting the | ||
| // service#threadGroupName would lead to a ThreadGroup being created each time the service is restarted. | ||
| String threadGroupName = getThreadGroupName(threadPoolName); | ||
| service.setNamePattern(threadGroupName + " - %t"); | ||
|
|
There was a problem hiding this comment.
I think this is the most problematic part - related to the ThreadFactoryResourceDefinition / ThreadFactoryService.
This resource allows to create custom thread factory, and allows user to define:
- thread group name,
- thread priority
- thread name pattern (which can use %G to emit thread group name).
The thread priority is not in fact set through a created ThreadGroup instance but directly on newly created Threads, so the ThreadGroup still doesn't have much meaning.
Above is a workaround - do not set the threadGroupName on the ThreadFactoryService, so no ThreadGroup is going to be created, instead use the thread group name as part of the name pattern.
If we have a freedom change the resource model, we could remove the threadGroupName attribute from the ThreadFactoryResource and just leave the name pattern and priority attributes, which still allows for the same end effect (thread group name can be "hard-coded" part of the pattern).
There was a problem hiding this comment.
@TomasHofman What you've done here is fine, except please add @deprecated(forRemoval="true") to AbstractThreadFactoryResolver.getThreadGroupName
Also, I think ThreadFactoryService should cache any ThreadGroup created in start in an instance field and call ThreadGroup.destroy() on it in stop(). Catch and ignore or DEBUG log any RuntimeException thrown from destroy(). If whatever service injected the ThreadFactoryService didn't clean up in its own stop() the threads it created, the destroy() call will fail, but oh well. Lets not fail or make log noise but we can debug log. If they did clean up, destroy() will clean up the parent TG ref to the group.
The various 'xxxThreadPoolService' classes in this module clean up their threads correctly.
There was a problem hiding this comment.
@bstansberry @TomasHofman threadGroup.destroy() is deprecated for removal on JDK 17 so I discourage their usage, in addition, it is not a reliable method. I agree with moving it to a variable instance since there is a 1:1 relationship between the service and the ThreadGroup, at first glance, I am expecting that a service reload can reuse the same ThreadGroup.
I commented on the JBoss List as well, the removal of the ThreadsGroups, although it looks like the way to follow since this class has been degraded in JDK, seems that it could be helpful for debugging purposes, so I am more inclined to keep them on the services that require it as a static field instead of remove them altogether. At least, don't do it until there is any other available option on the JDK to group them.
Thoughts?
There was a problem hiding this comment.
Hi @yersan, I did see the ticket you referred on the mailing list (https://bugs.openjdk.org/browse/JDK-8252885), it mentions some debugging related APIs like JDWP, JVM TI and JDI which I think is what you meant. It's sort of like these APIs contains some methods that lets you enumerate threads and subgroups belonging to a ThreadGroup.
My perspective is that we would not loose anything that much important. If you can't filter by groups you can still filter by name. The tools that I use don't support this anyway, or I didn't notice because I never felt the need to use it... But I know this is very subjective statement.
Regarding keeping an instance field in the ThreadFactoryService for created group, I think that only makes sense if we want to call the destroy method on it. The ThreadFactoryService instances are created fresh after each app server reload, so it won't help with reusing the original groups. If we want to reuse the original groups we would have to keep them in some some static map with service name or something like that as a key, or we would need to keep the ThreadFactoryService instance itself in a static field wherever they are created (and then have the instance field for the group).
The destroy method is still present in the JDK 21, I'm now checking, but it is an empty method. I'm slightly in favor of not using it, but I will go by what you guys like most.
There was a problem hiding this comment.
I'll defer to @yersan re the static ThreadGroups.
I don't see anything useful IDEA does with them; it just displays the TG name as part of its string representation of a thread. I don't see any filtering or sorting feature. I pinged @ehsavoie to ask if NetBeans does anything.
JConsole also doesn't do anything (i.e. the platform mbeans don't show TG data.)
If destroy() is a no-op in SE 21, then +1 to not using it. It being deprecated in 17 is good enough reason to drop it. And I agree if we don't call destroy() using an instance field does nothing. So for that we either live with the leak or add a static WeakHashMap<String, ThreadGroup> to cache them.
There was a problem hiding this comment.
NetBeans shows threads grouped under their thread group in the debug window
There was a problem hiding this comment.
In eclipse IDE you can group the threads by ThreadGroup in your debug view, so hence the utility for keeping the ThreadGroups if possible but removing the leak.
|
An ongoing discussion about this is available at: https://lists.jboss.org/archives/list/wildfly-dev@lists.jboss.org/thread/2NICNYJE4NO2HK67DOFTPZUGNAKD5RRM/ |
Right @TomasHofman, I wanted to highlight such feedback since developers could still see useful continue getting the threads grouped.
Thanks, right, I see what you mean. Looking at it I have also noticed that the Threads extension was deprecated and converted into a read-only extension to give support on mixed domains. However, that was even much before WF23, and we do not support mixed domains below WF23. @bstansberry Should not we remove completely this extension? If the above is true, by moving the Threads extension outside of the discussion, we can:
So far I am still in favor of 1. if possible. I have not checked each of the affected services yet. |
|
@yersan what do you mean exactly by "Threads extension" and it being deprecated? Where can I see that info? As far as I can see the classes (ThreadFactoryResolver erc) are being actively used by several subsystems (Batch, EJB), even in standalone mode. |
|
I probably need to retract my proposal to keep thread groups statically :). The thread-factory resource allows to modify thread group name, in which case you need to create new ThreadGroups... |
TomasHofman
force-pushed
the
WFCORE-6505
branch
from
0ca20b8
to
09016da
Compare
|
I updated the PR according to Brian's feedback, I think now it's the best idea. |
See ThreadsExtension
Ok, I need to be more familiarized with this subsystem to give a more accurate opinion; I only saw that |
TomasHofman
force-pushed
the
WFCORE-6505
branch
3 times, most recently
from
43d5cc5
to
fb11f5b
Compare
|
PR updated - thread groups are now kept in static fields. |
TomasHofman
force-pushed
the
WFCORE-6505
branch
from
fb11f5b
to
15cd7a2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
TomasHofman
force-pushed
the
WFCORE-6505
branch
from
15cd7a2
to
4ad8f23
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Core -> Full Integration Build 13095 outcome was FAILURE using a merge of 4ad8f23 Failed tests |
|
Core -> Full Integration Build 13096 outcome was FAILURE using a merge of 4ad8f23 Failed tests |
TomasHofman
force-pushed
the
WFCORE-6505
branch
from
4ad8f23
to
91e07e4
Compare
|
@yersan The threads module at this point is essentially a utility library used by other subsystems (Batch, EJB) to support configuration of thread pools and factories within their own config. ThreadFactoryService and its ThreadGroup creation is relevant for that reason. Some of what's in the module could be removed, yes, i.e. anything not related to its use as a utility library. That would be out of scope for this though. :) |
bstansberry
approved these changes
Oct 3, 2023
There was a problem hiding this comment.
@yersan This LGTM. Other than ThreadFactoryService, which needs to create dynamically named TGs, all the others are stored in constants, so that addresses your point re debuggers.
The ThreadFactoryService ones are cached in a static map, which solves the 'leak on reload' problem.
yersan
approved these changes
Oct 4, 2023
yersan
added
the
ready-for-merge
|
Thanks @TomasHofman and @bstansberry / @ehsavoie for helping with this |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.redhat.com/browse/WFCORE-6505
This is wildfly-core part of eliminating duplicate thread groups creation.
I tried to remove the thread groups and instead inject the group name string straight into the name pattern, where it was used by ("%G - ...").