-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
@RunOnVirtualThread cannot be used in override methods #36146
Comments
cc @cescoffier |
Virtual threads are unnamed by default, which explains the behavior. However, in the last version of Quarkus we are computing a unique name to simplify debuggability. For example, using Quarkus 3.4.1 with Java 21, I get:
As you can see, the thread name is present in the log message. I use your log format. Can you try with Java 21 and if it still does not work, provide a complete reproducer? |
@cescoffier I have just run with Java 21 (removing preview flag, too), but it doesn't work. https://github.com/GDG-Pistoia/virtual-kafka-streams Here there is full code to reproduce the issue. It is enough to run with http://localhost:8080/myapi/virtual the result inside log is still
And inside the |
Also, it would be great to reduce the size of the reproducer to the smallest possible code to reproduce an issue. |
Using the annotation in the interface does not work, you will have to restructure your code to use regular HTTP resources. |
Actually, we do:
But it only checks method / class, not the Java inheritance. |
I discussed it with @geoand and @Ladicek. First, there are rules defining how annotations are inherited in jax-rs. We need to check, but we are not sure it covers the current use case. After looking more deeply to the code with @geoand, supporting this specific case would require a large surgery. The virtual thread annotation is not the only one unsupported in this case. Almost no annotation would work like this, unfortunately. So we will check the Jax-rs rules and see what can be done. |
Thank you very much @cescoffier, with your advice using regular http method now it's working:
I beg your pardon for a huge code to reproduce the issue, next time I will provide a smaller one removing stuff. Back to the case, the first thing I thought about this new feature is provide API interface to be implemented, so the vendor can choose his own implementation inside the class that implements. This is a common use case when for example you define YAML openapi specs, generate java code and then you implement an interface directly or using proxy pattern. For these kind of scenario, every code based on this workflow should be edit code generation in order to provide Java code with @RunOnVirtualThread. But if inheritance worked, there would be no breaking changes to do (from client integration prospective). For example
This is a generated signature from YAML Openapi specs, then you have a delegate (proxy) implementation. Of course a concreate bean of RequestApi must exists
(I don't assure that this code can compile, it's simple report of a real working production code just to explain my point) This is done when you have contract based approach inside development workflow and you have yet exposed generated API to client for integration and you would like avoid to provide a breaking release (this happen every time inside a big company you have N applications that integrates your API but you don't have budget to cover a new API signature changes) I guess this could be block fast adoption of this feature for yet existing application |
That’s interesting! Why generator are you using? |
openapi-generator-maven-plugin |
Line 826 in 29181b3
The problem is more difficult than it might seem initially, you'll need a way to assiociate the interface to the implementation and then "merge" the annotations. |
According to what @Ladicek mentioned on Zulip, it might not even be "legal", as interface annotations should not be inherited. If that's the case, the approach used by the generator is not correct (and basically works by accident).
@Ladicek can you confirm? |
How annotation inheritance in Java works is described here: https://docs.oracle.com/javase/8/docs/api/java/lang/annotation/Inherited.html Notably, annotations are only inherited on classes (not on fields or methods), and only from superclasses (not from interfaces). How annotation inheritance in JAX-RS works is described here: https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-3.1#annotationinheritance Notably, it only concerns inheritance of JAX-RS annotations on methods and method parameters. The way I read it, it means that non-JAX-RS annotations are never inherited and should be present on the resource method. So if I understand the present issue correctly, we have a resource class with a resource method that inherits JAX-RS annotations from an interface method and then adds |
A possible workaround is given by the following statement from the Jakarta REST specification:
You can just copy the JAX-RS annotations from the interface to the resource method and then add |
@Ladicek I'm going to test this workaround, thanks! |
@Ladicek The workaround does work indeed. But that would mean moving all the Jax-rs annotations to the resource method. @Path("/hello")
@RunOnVirtualThread
public class GreetingApiDelegate implements GreetingApi {
@GET
@Override
public String hello() {
return String.format("Hello from RESTEasy Reactive: %s", Thread.currentThread());
}
} With the above changes, the operation will be executed on virtual thread otherwise on the |
Looking into GRPC support, on the first site, it seems the similar case is actually supported, but I wonder if this is due to having @GrpcService annotation on the class, like annotation copies are necessary in this case. In my experience, many prefer using interfaces to separate specification/documentation of a service, from an actual implementation. It increases readability and maintainability. In our case, we generate documentation based on interface definition, therefore we'd need to completely move away from this practice and put everything in the resource class. I'm aware of having documentation generation if jax-rs annotations are put in an implementation class. One more interesting thing I noticed is that the full URL isn't displayed in Intellij when interface is used which implies Quarkus doesn't support specification/implementation separation by default like Spring REST. I agree with @Ladicek that @RunOnVirtualThread doesn't make sense to be supported on interface level, since it's implementation specific, but think it's a bug that it isn't supported for annotations on interface level. |
Describe the bug
Inside a Quarkus 3.4.1 application, I cannot log virtual threads to verify virtual threads usage for a simple demo, using
@RunOnVirtualThread
.I am running the app with /Library/Java/JavaVirtualMachines/corretto-19.0.2/Contents/Home/bin/java (Java 19)
Inside maven-compiler-plugin I have this configuration
Like said inside https://quarkus.io/guides/virtual-threads#terminology guide.
I have
These log properties
With this configuration I would like to log if a method is running with virtual threads with quarkus-virtual-thread- prefix, but when I run a REST call to method annotated with @RunOnVirtualThread, it doesn't log virtual threads. The method is
How can I verify that actual virtual threads is running?
Expected behavior
I'm expecting like this
or something like this to verify actual virtual threads instead platform threads
Actual behavior
Actual log is
How to Reproduce?
No response
Output of
uname -a
orver
Darwin Lorenzos-MacBook-Pro.local 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:39:46 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T6020 arm64
Output of
java -version
java 19
GraalVM version (if different from Java)
No response
Quarkus version or git rev
3.4.1
Build tool (ie. output of
mvnw --version
orgradlew --version
)No response
Additional information
My laptop is MacOs with ARM architecture:
Darwin Lorenzos-MacBook-Pro.local 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:39:46 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T6020 arm64
Configuration of app
The text was updated successfully, but these errors were encountered: