[ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block. #2517
Conversation
…leged block. * Ensuring that all threadPoolFactories are created under a privileged block so the threads created are under the same AccessControlContext. Jira: https://issues.apache.org/jira/browse/ARTEMIS-2171
| @Override | ||
| public Thread newThread(Runnable r) { | ||
| return new Thread(r); | ||
| public ThreadFactory run() { |
There was a problem hiding this comment.
I know this inst your change, but it highlights a bigger question, as to why is this not using ActiveMQThreadFactory?
As this creates threads with the captured context.
There was a problem hiding this comment.
Because i looked at all threadFactories that werre not protected, didn't check the implementation. Sorry for this noise on ActiveMQThreadFactory
| return AccessController.doPrivileged(new PrivilegedAction<ActiveMQThreadFactory>() { | ||
| @Override | ||
| public ActiveMQThreadFactory run() { | ||
| return new ActiveMQThreadFactory(this.getClass().getSimpleName() + "-scheduled-threads", false, getThisClassLoader()); |
There was a problem hiding this comment.
Threads created by ActiveMQThreadFactory should be already created by doPrivileged. how are we sure its not just that the thread issues you are seeing are maybe just where code isn't using ActiveMQThreadFactory
see in ActiveMQThreadFactory
@Override
public Thread newThread(final Runnable command) {
// create a thread in a privileged block if running with Security Manager
if (acc != null) {
return AccessController.doPrivileged(new ThreadCreateAction(command), acc);
} else {
return createThread(command);
}
}
There was a problem hiding this comment.
Then why do you still 'protect' ActiveMQThreadFactory like in https://github.com/apache/activemq-artemis/blob/master/artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java#L350 maybe those blocks should go away too ?
There was a problem hiding this comment.
As i said if you can supply a test with this PR to recreate the issue. then we can validate what does or what does not need to be changed.
E.g. if it is truely needed then may look to put the logic in one common place.
And like wise avoid future breakage should new code be added or existing refactored
The fact i believe wildfly has been using 1.5 of artemis without issue and the same thread factory is there. Its important that the issue is well understood, and avoid future regression
There was a problem hiding this comment.
Hum, I think the issue was initially reported against 1.5.x since it is almost year old but i don't have much details in the initial bug report.
There was a problem hiding this comment.
I cannot stress this enough. Test first please.
There was a problem hiding this comment.
I'm trying to 'see' the issue within WildFly so I can provide a test
| thr.setContextClassLoader(moduleTccl); | ||
| return null; | ||
| public ThreadFactory run() { | ||
| return new ThreadFactory() { |
There was a problem hiding this comment.
I know this inst your change, but it highlights a bigger question, as to why is this not using ActiveMQThreadFactory?
As this creates threads with the captured context.
|
This really needs a test case A) validate the fix (and validate any alternative proposals) |
| @@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String addressList) { | |||
|
|
|||
| @Override | |||
| protected ActiveMQThreadFactory getThreadFactory() { | |||
| return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-", false, getThisClassLoader()); | |||
There was a problem hiding this comment.
This is just creating a ThreadFactory.. is it really needed?
I would expect a security check around new Threads, but against a new ActiveMQThreadFactory?
are you sure about it? just checking it's not a typo?
There was a problem hiding this comment.
I'd like to see a test case on this. I think the issue might be we have some ThreadFactories which are not using the ActiveMQThreadFactory which should imo ensure the privileged access if its used in replacement for those, and actually we just need to fix those up. A test though is the best way for sure, so what ever fix, we know its resolved.
There was a problem hiding this comment.
We run WildFly test suite with a security manager on (which is how this leak was found) BUT I don't have the runs where this happened so I don't know the symptoms :( for a test ensure that this is resolved.
There was a problem hiding this comment.
I mean we need a test in ActiveMQ Artemis for this to merge.
We need to be able to build, refactor, and release with knowing we haven't broken something our users and upstream/dependent projects have on us. Like wise we need to validate the fix.
essentially i suggest a test is created with a security manager on, in the activemq artemis project, with a similar leak detector to what youre doing in your project "wildfly" so to create this testing scenario in artemis.
There was a problem hiding this comment.
I see the number of ActiveMQThreadFactory$1 increasing steadily without any activity on current master of WildFly with Artemis 2.6.4
There was a problem hiding this comment.
I don't see it on WildFly with 2.6.4 if I wait sufficiently for the polls to decrease
There was a problem hiding this comment.
I think the increase stops when the default size is reached so this doesn't look like an issue
There was a problem hiding this comment.
Great stuff theres no leak.
That issue of it increasing the pool to max even if idle should be resolved in next release 2.7.0
There was a problem hiding this comment.
@ehsavoie can you close this PR then? And the Jira you opened.
There was a problem hiding this comment.
@ehsavoie please close this then.
block so the threads created are under the same AccessControlContext.
Jira: https://issues.apache.org/jira/browse/ARTEMIS-2171