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.

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

@ThreadContextConfig does not work in Quarkus #22645

Closed
rvansa opened this issue Jan 5, 2022 · 10 comments · Fixed by #23576
Closed

@ThreadContextConfig does not work in Quarkus #22645

rvansa opened this issue Jan 5, 2022 · 10 comments · Fixed by #23576
Assignees
Labels
Milestone

Comments

@rvansa
Copy link
Contributor

rvansa commented Jan 5, 2022

Describe the bug

While it is documented in https://quarkus.io/guides/context-propagation annotating

@Inject
@ThreadContextConfg(cleared = [ ... ])
ThreadContext threadContext;

does not work - the annotation seems to be ignored and all context is propagated.

Expected behavior

This annotation should work as documented in the guide https://quarkus.io/guides/context-propagation , e.g. not propagate transaction context when this is supposed to be cleared.

Actual behavior

All contexts are propagated.

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.3.0.Final

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

Workaround exists:

SmallRyeContextManagerProvider.getManager().newThreadContextBuilder()
            .propagated(ThreadContext.CDI, "JAX-RS", "Servlet").cleared(ThreadContext.TRANSACTION).build();
@rvansa rvansa added the kind/bug Something isn't working label Jan 5, 2022
@FroMage
Copy link
Member

FroMage commented Jan 5, 2022

@manovotn do you think you can try to make that work in Quarkus?

IIRC it used to work in SR as a CDI extension, which wasn't supported in ArC at the time. Perhaps that's changed, or we can find other ways to make it work?

@manovotn
Copy link
Contributor

manovotn commented Jan 5, 2022

Yea, this wasn't possible back then but should be doable now. We just didn't get to it since.

I won't be able to jump onto this right away but can add it to my TODO list.

The logic implementing this for pure MP is inside SmallryeContextCdiExtension so we'll need to add an equivalent using Quarkus tooling. The crux of it is that we need to read all the injection points which have @ThreadContextConfig and for those that don't have user-defined producers, we want to create a synthetic bean returning ManagedExecutor/ThreadContext with proper context propagation configuration.

Ccing @Ladicek and @mkouba just in case one of them feels like implementing this before I can get to it :)

@mkouba
Copy link
Contributor

mkouba commented Jan 5, 2022

OT: I wonder if we could add an area/context-propagation label? We had a lot of issues related to this topic so far.

@rvansa
Copy link
Contributor Author

rvansa commented Jan 5, 2022

Btw. the javadoc mentions using @NamedInstance along with @ThreadContextConfig - what's the point of this? When I tried to use this it told me that there's no matching ThreadContext. Should @ThreadContextConfig create a synthetic producer that will provide instances with this name, and the annotation is there for sharing the ThreadContext across beans?

@manovotn
Copy link
Contributor

manovotn commented Jan 5, 2022

When I tried to use this it told me that there's no matching ThreadContext.

This won't work in Quarkus as it's not implemented same as the original annotation. This is because @NamedInstance is just an optional improvement for @ThreadContextConfig.

Should @ThreadContextConfig create a synthetic producer that will provide instances with this name, and the annotation is there for sharing the ThreadContext across beans?

@rvansa almost spot on - the javadoc of NamedInstance attempts to explain it.
@ThreadContextConfig creates a synthetic bean (not producer) for every injection point and to prevent ambiguity, it modifies all injection points by adding @NamedInstance CDI qualifier with a generated name (e.g. annotation value). By declaring this qualifier yourself, you control the name and therefore can effectively share those generated instance throughout your code (by default, no sharing happens). All injection point declaring the same name obviously need to have the same configuration.
Does this explanation make sense? Ask away if not ;-)

@gsmet
Copy link
Member

gsmet commented Jan 5, 2022

@mkouba I created area/context-propagation . I'll let you add the rules you want here: https://github.com/quarkusio/quarkus/blob/main/.github/quarkus-bot.yml . Even if you don't add a pattern for the title, it's still useful to have the bot ping the appropriate persons when we add the label and remove the needs-triage one.

@mkouba
Copy link
Contributor

mkouba commented Jan 5, 2022

@gsmet done: #22661

@rvansa
Copy link
Contributor Author

rvansa commented Jan 5, 2022

@manovotn Thanks, so I got the failure because the @ThreadContextConfig was effectively ignored and the @NamedInstance was only interpreted as receiver.

I personally find a bit confusing the I'd have @Inject @NamedInstance("foo") ThreadContext tc once with the @ThreadContextConfig annotation and once without but I guess it's out of scope of Quarkus to change that. Actually - it seems that this annotation can be applied to a type, too - what's the purpose of that? I can imagine declaring the bean as

@ThreadContextConfig(...)
public interface MyCustomThreadContext extends ThreadContext {}

@manovotn
Copy link
Contributor

manovotn commented Jan 6, 2022

Thanks, so I got the failure because the @ThreadContextConfig was effectively ignored and the @NamedInstance was only interpreted as receiver.

Yes

Actually - it seems that this annotation can be applied to a type, too

That seems like an oversight, I don't really see any realistic usage for that.

I personally find a bit confusing the I'd have @Inject @NamedInstance("foo") ThreadContext tc once with the @ThreadContextConfig annotation and once without but I guess it's out of scope of Quarkus to change that.

This feature is not within the scope of MP specification so we can change it. I simply implemented this as a nice to have feature that optionally enables sharing of instances but to have it in CDI-friendly way, this was probably the best I could come up with back in the day.

@manovotn manovotn self-assigned this Feb 8, 2022
@manovotn
Copy link
Contributor

manovotn commented Feb 8, 2022

I will look into implementing this
I also stumled upon a very old issue where we apparently already knew this was missing but it got lost - #6156

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants