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

VirtualThreads cannot be used with JAX-RS interfaces #45186

Open
danielbobbert opened this issue Dec 18, 2024 · 16 comments · May be fixed by #45202
Open

VirtualThreads cannot be used with JAX-RS interfaces #45186

danielbobbert opened this issue Dec 18, 2024 · 16 comments · May be fixed by #45202
Labels
area/rest kind/bug Something isn't working

Comments

@danielbobbert
Copy link
Contributor

Describe the bug

We are implementing a JAX-RS endpoint, where the API is given by a Java interface (provided by another project). Our implementation consists of an ApplicationScoped bean implementing this interface. We would like to have the implementation execute on virtual threads, but Quarkus fails to recognize the annotation.

Expected behavior

We should be able to put @RunOnVirtualThread on the class or methods of our bean that implements the JAX-RS interface, to have them execute on a virtual thread.

Actual behavior

Putting @RunOnVirtualThread

  • on the interface itself: ignored
  • on a method of the interface: ignored
  • on the implementing bean itself: ignored
  • on a method of the implementing bean: build time error because the annotation is only allowed on "entrypoint methods"

There seems to be no valid way to get one of the methods to execute on a virtual thread.
This behaviour is present in Quarkus 3.17.4 but was already present in 3.8 (with quarkus-resteasy-reactive), so it seems to be general issue with the recognition of "entrypoints" in conjunction with Java interfaces.

How to Reproduce?

I have attached a small reproducer tha shows that @RunOnVirtualThread works correctly when put on a JAX-RS bean, but it is being ignored (or leads to an error message) when used in conjunction with interfaces.

Output of uname -a or ver

Mac OS

Output of java -version

Java 21

Quarkus version or git rev

3.17.4

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

mvn 3.9.9

Additional information

virtual-rest-impl.zip

@danielbobbert danielbobbert added the kind/bug Something isn't working label Dec 18, 2024
Copy link

quarkus-bot bot commented Dec 18, 2024

/cc @FroMage (rest), @stuartwdouglas (rest)

@geoand
Copy link
Contributor

geoand commented Dec 18, 2024

Thanks for reporting.

I am pretty sure we've discussed this before. Do you remember @michalvavrik @cescoffier @Ladicek ?

@cescoffier
Copy link
Member

Yes, their was discussed. If I remember correctly, we said we did not want to support @RunOnVirtualThread on interfaces.

@cescoffier
Copy link
Member

Actually the report is slightly different. Not sure how it can be fixed...

@danielbobbert
Copy link
Contributor Author

IMHO, the annotation doesn't belong on the interface (as it is an implementation detail). What I am asking for is that the annotation be allowed and honoured on the implementing bean.
Any way to set a default would also help (akin to "quarkus.messaging.blocking.signatures.execution.mode" which allows to set a default for message consumers).
One can already annotate a JAX-RS "Application" class with @Blocking or @nonblocking. Allowing @RunOnVirtualThread there to set the "BlockingDefault" on the "ApplicationScanningResult" would also be an option.

I am aware of the risks of setting virtual threads as the default (that's why I am asking for annoating the implementing bean). But right now, there seems to be no way at all. So setting a default (where it is known to be safe) would still be better than nothing...

@danielbobbert
Copy link
Contributor Author

I guess, the proper "fix" would be to check implemented interfaces for JAX-RS annotations when collecting "entrypoint" methods. Not sure how complicated that could become, and maybe it's OK to prohibit multiple inheritance and such. But the plain case of "Bean implements Intfce" where the JAX-RS annotations are on the interface would be really helpful (and not an uncommon case for "API-first" development cases).

@geoand
Copy link
Contributor

geoand commented Dec 18, 2024

I'll have a look

@geoand
Copy link
Contributor

geoand commented Dec 18, 2024

Actually cc @Ladicek who did the implementation of ExecutionModelAnnotationsProcessor

@danielbobbert
Copy link
Contributor Author

Well, the ExecutionModelAnnotationsProcessor only covers the "symptoms". Even if I disable the checking by setting the "detection-mode" to "disabled" I cannot get the methods to execute on a virtual thread, no matter where I put the annotation (class vs method, bean vs interface). So I think the problem is rather in the area of detecting endpoint methods. I wonder whether org.jboss.resteasy.reactive.common.processor.EndpointIndexer.createResourceMethod() would be a good point to look at?!

@danielbobbert
Copy link
Contributor Author

danielbobbert commented Dec 18, 2024

I think, I found one (other?) part of the problem, while debugging through EndpointIndexer.isRunOnVirtualThread().
This is called twice from createResourceMethod(): once for the "currentMethodInfo" and once for the "actualMethodInfo" (which differ in the case of "bean implements interface").
Now in the first case, the "default" passed to isRunOnVirtualThread() is "AUTOMATIC". But in the second case (actualMethodInfo), "BLOCKING" is passed (see explicit comment in line 738). But the way isRunOnVirtualThread() is written, this default actually overwrites the explicit recognition of the annotation.

@danielbobbert
Copy link
Contributor Author

In my testing, when passing AUTOMATIC to the second (actualMethodInfo) invocation of isRunOnVirtualThread(), then the annotation seems to be honoured correctly on the implementing bean (both on class and method level).
To be honest, I don't really understand the comment in line 738. Is this supposed to be a performance optimization? I can't spot anything in isBlocking() and isRunOnVirtualThread() that would make a difference though (besides maybe doesMethodHaveBlockingSignature()being called at the end of isBlocking().

@danielbobbert
Copy link
Contributor Author

I am wondering what the intended semantics of the "blockingDefault" is. My current understanding is (in the given order) of preference:

  • If a method is explicitly annotated with @Blocking, @nonblocking or @RunOnVirtualThread, use that (and bail out if the annotation conflicts with the signature)
  • If the class is explicitly annotated use that.
  • If an application default is given (other than AUTOMATIC) use that
  • derive from the method signature

Or in short: "explicit annotation beats default"

Is that understanding correct? The method isBlocking()also seems to confirm this understanding, while isRunOnVirtualThread() looks more like "if a default was configured, always use that. Otherwise look at annotations and signatures".

Also, isRunOnVirtualThread() internally calls isBlocking()again. It might make sense to just pass the result of previous call to isBlocking() into isRunOnVirtualThread() for reuse (they are always called in tandem by createResourceMethod()).

@geoand
Copy link
Contributor

geoand commented Dec 19, 2024

Or in short: "explicit annotation beats default"

This is indeed the expected behavior.

@danielbobbert
Copy link
Contributor Author

Will you be fixing this or do you want me to create a PR?

@geoand
Copy link
Contributor

geoand commented Dec 19, 2024

I'm busy with other stuff ATM, so if you have a fix in mind, feel free to open a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/bug Something isn't working
Projects
None yet
3 participants