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

@RunOnVirtualThread cannot be used in override methods #36146

Closed
poldinik opened this issue Sep 25, 2023 · 21 comments
Closed

@RunOnVirtualThread cannot be used in override methods #36146

poldinik opened this issue Sep 25, 2023 · 21 comments
Labels
kind/bug Something isn't working

Comments

@poldinik
Copy link
Contributor

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

<plugin>
    <artifactId>maven-compiler-plugin</artifactId>
    <version>${compiler-plugin.version}</version>
    <configuration>
        <compilerArgs>
            <arg>--enable-preview</arg>
            <arg>-parameters</arg>
        </compilerArgs>
    </configuration>
</plugin>

Like said inside https://quarkus.io/guides/virtual-threads#terminology guide.

I have

<dependency>
    <groupId>io.quarkus</groupId>
    <artifactId>quarkus-resteasy-reactive-jackson</artifactId>
</dependency>

These log properties

# Log
quarkus.log.console.format=%d{yyyy-MM-dd HH:mm:ss,SSS} %-5p [%c{2.}] %h %H ${quarkus.http.port} %i (%t) %s%e%n
quarkus.log.console.level=INFO
quarkus.log.file.enable=false
#quarkus.log.file.path=/tmp/bile.log
quarkus.log.file.format=%d{yyyy-MM-dd HH:mm:ss,SSS} %-5p [%c{2.}] %h %H ${quarkus.http.port} %i (%t) %s%e%n

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

@Override
    @RunOnVirtualThread
    public Response reactive() {
        log.info("operationName=reactive");
        // Runs on the event loop
        return Response.ok(Map.of("msg", Thread.currentThread())).build();
    }

How can I verify that actual virtual threads is running?

Expected behavior

I'm expecting like this

2023-09-25 22:48:13,539 INFO  [it.gd.bo.MyApiImpl] lorenzos-macbook-pro lorenzos-macbook-pro.local 8083 15051 (executor-quarkus-virtual-thread-5) operationName=reactive

or something like this to verify actual virtual threads instead platform threads

Actual behavior

Actual log is

2023-09-25 22:48:13,539 INFO  [it.gd.bo.MyApiImpl] lorenzos-macbook-pro lorenzos-macbook-pro.local 8083 15051 (executor-thread-5) operationName=reactive

How to Reproduce?

No response

Output of uname -a or ver

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 or gradlew --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

<?xml version="1.0" encoding="UTF-8"?>

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>

  <groupId>it.gdgpt</groupId>
  <artifactId>virtual-kafka-streams</artifactId>
  <version>1.0.0-SNAPSHOT</version>
  <packaging>pom</packaging>

  <properties>
    <compiler-plugin.version>3.10.1</compiler-plugin.version>
    <maven.compiler.source>19</maven.compiler.source>
    <maven.compiler.target>19</maven.compiler.target>
    <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
    <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
    <quarkus.platform.artifact-id>quarkus-bom</quarkus.platform.artifact-id>
    <quarkus.platform.group-id>io.quarkus.platform</quarkus.platform.group-id>
    <quarkus.platform.version>3.4.1</quarkus.platform.version>
    <skipITs>true</skipITs>
    <surefire-plugin.version>3.0.0-M7</surefire-plugin.version>
    <npg-be-ark-library.version>0.0.2-java-1.8-SNAPSHOT</npg-be-ark-library.version>
  </properties>
  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>${quarkus.platform.group-id}</groupId>
        <artifactId>${quarkus.platform.artifact-id}</artifactId>
        <version>${quarkus.platform.version}</version>
        <type>pom</type>
        <scope>import</scope>
      </dependency>
    </dependencies>
  </dependencyManagement>

  <modules>
    <module>virtual-kafka-streams-liquibase</module>
    <module>virtual-kafka-streams-application</module>
  </modules>
</project>

<?xml version="1.0" encoding="UTF-8"?>
        <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>

    <parent>
        <groupId>it.gdgpt</groupId>
        <artifactId>virtual-kafka-streams</artifactId>
        <version>1.0.0-SNAPSHOT</version>
    </parent>

    <artifactId>virtual-kafka-streams-application</artifactId>
    <version>1.0.0-SNAPSHOT</version>

    <dependencies>
<!--        <dependency>-->
<!--            <groupId>io.quarkus</groupId>-->
<!--            <artifactId>quarkus-smallrye-reactive-messaging-kafka</artifactId>-->
<!--        </dependency>-->
<!--        <dependency>-->
<!--            <groupId>io.quarkus</groupId>-->
<!--            <artifactId>quarkus-kafka-streams</artifactId>-->
<!--        </dependency>-->
        <dependency>
            <groupId>io.vavr</groupId>
            <artifactId>vavr</artifactId>
            <version>0.10.4</version>
        </dependency>
        <dependency>
            <groupId>org.projectlombok</groupId>
            <artifactId>lombok</artifactId>
            <version>1.18.24</version>
            <scope>provided</scope>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-hibernate-orm</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-rest-client-reactive</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-smallrye-openapi</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-quartz</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-scheduler</artifactId>
        </dependency>
<!--        <dependency>-->
<!--            <groupId>io.quarkus</groupId>-->
<!--            <artifactId>quarkus-resteasy-jackson</artifactId>-->
<!--        </dependency>-->
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-resteasy-reactive-jackson</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-smallrye-health</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-jdbc-postgresql</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-jdbc-h2</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-arc</artifactId>
        </dependency>
<!--        <dependency>-->
<!--            <groupId>io.quarkus</groupId>-->
<!--            <artifactId>quarkus-resteasy</artifactId>-->
<!--        </dependency>-->
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-junit5</artifactId>
            <scope>test</scope>
        </dependency>
        <dependency>
            <groupId>io.rest-assured</groupId>
            <artifactId>rest-assured</artifactId>
            <scope>test</scope>
        </dependency>
    </dependencies>
    <build>
        <plugins>
            <plugin>
                <groupId>${quarkus.platform.group-id}</groupId>
                <artifactId>quarkus-maven-plugin</artifactId>
                <version>${quarkus.platform.version}</version>
                <extensions>true</extensions>
                <executions>
                    <execution>
                        <goals>
                            <goal>build</goal>
                            <goal>generate-code</goal>
                            <goal>generate-code-tests</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>
            <plugin>
                <artifactId>maven-compiler-plugin</artifactId>
                <version>${compiler-plugin.version}</version>
                <configuration>
                    <compilerArgs>
                        <arg>--enable-preview</arg>
                        <arg>-parameters</arg>
                    </compilerArgs>
                </configuration>
            </plugin>
            <plugin>
                <artifactId>maven-surefire-plugin</artifactId>
                <version>${surefire-plugin.version}</version>
                <configuration>
                    <systemPropertyVariables>
                        <java.util.logging.manager>org.jboss.logmanager.LogManager</java.util.logging.manager>
                        <maven.home>${maven.home}</maven.home>
                    </systemPropertyVariables>
                </configuration>
            </plugin>
            <plugin>
                <artifactId>maven-failsafe-plugin</artifactId>
                <version>${surefire-plugin.version}</version>
                <executions>
                    <execution>
                        <goals>
                            <goal>integration-test</goal>
                            <goal>verify</goal>
                        </goals>
                        <configuration>
                            <systemPropertyVariables>
                                <native.image.path>${project.build.directory}/${project.build.finalName}-runner</native.image.path>
                                <java.util.logging.manager>org.jboss.logmanager.LogManager</java.util.logging.manager>
                                <maven.home>${maven.home}</maven.home>
                            </systemPropertyVariables>
                        </configuration>
                    </execution>
                </executions>
            </plugin>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-compiler-plugin</artifactId>
                <configuration>
                    <source>19</source>
                    <target>19</target>
                </configuration>
            </plugin>
        </plugins>
    </build>
    <profiles>
        <profile>
            <id>native</id>
            <activation>
                <property>
                    <name>native</name>
                </property>
            </activation>
            <properties>
                <skipITs>false</skipITs>
                <quarkus.package.type>native</quarkus.package.type>
            </properties>
        </profile>
    </profiles>

</project>
@poldinik poldinik added the kind/bug Something isn't working label Sep 25, 2023
@poldinik poldinik changed the title Virtual Threads Not Logged inside Quarkus with Java 19 Virtual Threads Not Logged inside Quarkus 3 with Java 19 Sep 25, 2023
@geoand
Copy link
Contributor

geoand commented Sep 26, 2023

cc @cescoffier

@cescoffier
Copy link
Member

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:

2023-09-26 08:22:59,762 INFO  [me.es.GreetingResource] cescoffi-mac cescoffi-mac 8080 28578 (quarkus-virtual-thread-0) operationName=hello

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 cescoffier added the triage/needs-reproducer We are waiting for a reproducer. label Sep 26, 2023
@poldinik
Copy link
Contributor Author

poldinik commented Sep 26, 2023

@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 mvn compile quarkus:dev inside virtual-kafka-streams-application submodule and then
calling

http://localhost:8080/myapi/virtual

the result inside log is still

lorenzos-macbook-pro lorenzos-macbook-pro.local 8080 17507 (executor-thread-1) operationName=virtual

And inside the Thread.currentThread(), the result has virtual: false

image

@cescoffier
Copy link
Member

@RunOnVirtualThread cannot be used on a bean, only on an HTTP method, @incoming method, @scheduled methods... Not on any bean methods. It's not supporting inheritance, so your MyApiImpl is not going to be invoked on a virtual thread.
Try to use @RunOnVirtualThread in the resource directly (even if I'm not sure if it will be working)

Also, it would be great to reduce the size of the reproducer to the smallest possible code to reproduce an issue.

@cescoffier
Copy link
Member

Using the annotation in the interface does not work, you will have to restructure your code to use regular HTTP resources.

@cescoffier
Copy link
Member

Actually, we do:

Map.Entry<AnnotationTarget, AnnotationInstance> runOnVirtualThreadAnnotation = getInheritableAnnotation(info,
                RUN_ON_VIRTUAL_THREAD);

But it only checks method / class, not the Java inheritance.

@cescoffier
Copy link
Member

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.

@cescoffier cescoffier changed the title Virtual Threads Not Logged inside Quarkus 3 with Java 19 @RunOnVirtualThread cannot be used in override methods Sep 26, 2023
@poldinik
Copy link
Contributor Author

poldinik commented Sep 26, 2023

Thank you very much @cescoffier, with your advice using regular http method now it's working:

2023-09-26 22:24:08,669 INFO [it.gd.bo.VirtualApi] lorenzos-macbook-pro lorenzos-macbook-pro.local 8080 24131 (quarkus-virtual-thread-0) operationName=virtual

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

@javax.annotation.Generated
@Api(value = "myrequest", description = "the request API")
public interface RequestApi {

    default RequestApiDelegate getDelegate() {
        return new RequestApiDelegate() {};
    }

   
    @ApiOperation(value = "My Request", nickname = "myRequest", notes = "API to test request", tags={  })
    @ApiResponses(value = { 
        @ApiResponse(code = 200, message = "Success"),
        @ApiResponse(code = 400, message = "Invalid request data", response = ClientError.class),
        @ApiResponse(code = 500, message = "Internal Server Error", response = ServerError.class) })
    @POST
    default Response myRequest(@ApiParam(value = "" ,required=true) @HeaderParam("correlationId") UUID correlationId,@ApiParam(value = "" ,required=true )  Request request) {
        return getDelegate().myRequest(correlationId, request);
    }

}

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

@RequestScoped
public class RequestApiDelegateImpl implements RequestApiDelegate {

    @Inject 
    BeanService service;

    @Override
    public Response myRequest(UUID correlationId, Request request) {
        service.somethingUsecase();
        return Response.ok().build()
    }

}

(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

@cescoffier
Copy link
Member

That’s interesting! Why generator are you using?

@geoand geoand removed the triage/needs-reproducer We are waiting for a reproducer. label Sep 27, 2023
@poldinik
Copy link
Contributor Author

openapi-generator-maven-plugin
If there are many fix do to in order to support inheritance, I would like to propose a contribution forking quarkus project. Which is the entry point inside code to start to develop? And which workflow should I follow to lead a proposal?

@geoand
Copy link
Contributor

geoand commented Sep 27, 2023

Map.Entry<AnnotationTarget, AnnotationInstance> runOnVirtualThreadAnnotation = getInheritableAnnotation(info,
is where we find the annotation.

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.

@cescoffier
Copy link
Member

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

the Java language supports inheriting annotations that are present on the class itself, and only from super-classes (not from super-interfaces), and only if they are meta-annotated @inherited, which is relatively bearable

@Ladicek can you confirm?

@Ladicek
Copy link
Contributor

Ladicek commented Sep 29, 2023

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 @RunOnVirtualThread on its own. I think it's fairly sensible to treat some of our annotations as JAX-RS annotations, but I don't feel like @RunOnVirtualThread should be one of those. It's not my call to make, but if you agree with my opinion, what we have here is a genuine bug.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 29, 2023

A possible workaround is given by the following statement from the Jakarta REST specification:

For consistency with other Jakarta EE specifications, it is recommended to always repeat annotations instead of relying on annotation inheritance.

You can just copy the JAX-RS annotations from the interface to the resource method and then add @RunOnVirtualThread.

@poldinik
Copy link
Contributor Author

poldinik commented Oct 5, 2023

@Ladicek I'm going to test this workaround, thanks!

@haroonsin
Copy link

@Ladicek The workaround does work indeed. But that would mean moving all the Jax-rs annotations to the resource method.
Would have loved @RunOnVirtualThread to follow through with the same approach.

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

@Lucky392
Copy link

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.

@gsmet
Copy link
Member

gsmet commented Nov 21, 2024

@Ladicek @geoand @cescoffier could we take a final decision here? My understanding is that we don't want to support inheriting @RunOnVirtualThread from interfaces? In this case, we should probably state it and close the issue as won't fix.

@geoand
Copy link
Contributor

geoand commented Nov 21, 2024

Sounds reasonable to me.

@geoand geoand closed this as completed Nov 21, 2024
@Ladicek
Copy link
Contributor

Ladicek commented Nov 21, 2024

My understanding of this issue (but it's been a while, so I might misremember), that's the exact problem here -- @RunOnVirtualThread is not present on the method in the interface, but on the method in the implementation class, and it is not honored. So if we don't want to support inheriting @RunOnVirtualThread from the interface's methods, this should not be closed.

Again, IIRC.

@cescoffier
Copy link
Member

I don't remember the exact case. But @RunOnVirtualThread was not intended to be inherited.

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

No branches or pull requests

7 participants