Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of the EventEmitter ThreadLocal #2133

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Get rid of the EventEmitter ThreadLocal #2133

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions server/implementation/src/main/java/io/smallrye/graphql/execution/event/EventEmitter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.smallrye.graphql.execution.event;

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Iterator;
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deployments share the same class loader?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the JAR is in a static module and not part of the deployment, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that in WildFly, threads are managed by a thread pool, so they are not shared between deployments. But I may be wrong.

Copy link
Member

@jmartisk jmartisk Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you make this into an SPI and the Quarkus extension uses the SPI to set up a single instance instead of a TL :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private final List<EventingService> enabledServices;

public static EventEmitter getInstance() {
return eventEmitters.get();
return INSTANCE;
}

private EventEmitter() {
Expand Down Expand Up @@ -70,7 +73,7 @@ private EventEmitter() {
}
}
enabledServices.sort(Comparator.comparing(this::getPriority));
this.enabledServices = enabledServices;
this.enabledServices = Collections.unmodifiableList(enabledServices);
}

private int getPriority(EventingService es) {
Expand Down
Loading