Get rid of the EventEmitter ThreadLocal #2133
Conversation
This is partly responsible from our class loader leaks.
| @@ -37,11 +38,13 @@ | |||
| */ | |||
| public class EventEmitter { | |||
| private static final Logger LOG = Logger.getLogger(EventEmitter.class); | |||
| private static final ThreadLocal<EventEmitter> eventEmitters = ThreadLocal.withInitial(EventEmitter::new); | |||
|
|
|||
| private static final EventEmitter INSTANCE = new EventEmitter(); | |||
There was a problem hiding this comment.
This most definitely breaks multi-deployment scenarios in wildfly, because all deployments will see the same instance of the emitter. But I assume that with a ThreadLocal it was also wrong, because threads in WF are shared between applications. I'm not sure what the correct solution is, should it be a map that is keyed by the TCCL of the deployment?
(We don't care too much about multi-deployment scenarios in wildfly, but I would like it at least to be not completely broken)
There was a problem hiding this comment.
The deployments share the same class loader?
There was a problem hiding this comment.
When the JAR is in a static module and not part of the deployment, yes.
There was a problem hiding this comment.
But don’t you have the same issue in Config where you load a static config from the service loader?
Or you expect the service loader to return the same thing for all deployments?
Aware of it because I created: https://github.com/smallrye/smallrye-graphql/pull/2132/files
(which doesn’t change the behavior I think).
There was a problem hiding this comment.
I thought that in WildFly, threads are managed by a thread pool, so they are not shared between deployments. But I may be wrong.
There was a problem hiding this comment.
I'm thinking that I might just completely give up on trying to make it work with multiple deployments. In the JBoss EAP XP product, it's explicitly unsupported, see https://access.redhat.com/articles/2026253
Only a single microservice app (deployment) on a given server is allowed. Multiple Deployments on a given server is not supported.
(not that SmallRye GraphQL is part of the product, of course, but it makes sense to keep the policies roughly the same)
I've been putting in very limited effort to get it working, but it gets annoying because you then have to avoid most usages of static fields etc.
I thought that in WildFly, threads are managed by a thread pool, so they are not shared between deployments. But I may be wrong.
It is a thread pool that is shared by deployments. Unless you configure a separate one for each deployment, which I think would be possible.
There was a problem hiding this comment.
@jmartisk I would say you have two options:
- either you go with this patch and forget about multi-deployment
- or you need a well defined bootstrap and the ability to propagate the instance where it's needed (what we do in HV)
In any case, the current situation is problematic: you keep the instances around and you create more than actually needed.
There was a problem hiding this comment.
Or you make this into an SPI and the Quarkus extension uses the SPI to set up a single instance instead of a TL :)
There was a problem hiding this comment.
Yeah I'll probably drop multi-deployment for now. I will keep it on the basic level (both GraphQL server and client deployments should work properly as long as they don't need to have different configuration values or eventing services). Maybe revisit it in the future, but it's annoying and time-consuming to verify and test that everything works in isolation.
So yeah, a simple static field will be fine I guess.
|
Same as the other ones, ideally, it would be nice to have it in Quarkus relatively fast. |
|
Yeah I'm on it |
Part of the reasons why
QuarkusClassLoaderinstances are kept around is theThreadLocalinEventEmitter.I'm not exactly sure why you are using a
ThreadLocalhere? The contract of the eventing services is not thread safe? I would expect them to be thread safe but I suppose there's a good reason.In any case, I experimented with removing it and it seems to improve things quite significantly.
Now I'm not entirely sure why this was implemented in the first place so I open the PR more for discussion than to get merged right away...
/cc @phillip-kruger @cescoffier @jmartisk