quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.64k stars 2.64k forks source link

Quarkus from 3.11 cannot load RestClient generated classes in Kotlin CoroutineScope #41349

Closed Sic4Parvis9Magna closed 3 months ago

Sic4Parvis9Magna commented 3 months ago

Describe the bug

The issue is the same as described in this closed ticket for Quarkus 3.9. This time I prepared a small project based on Quarkus documentation for the rest clients.

In short, Quarkus uses a different class loader in Kotlin CoroutineScope which results in some generated classes being missing.

Expected behavior

Quarkus uses the same class loader inside a coroutine scope as outside while working with generated rest clients. So code like this does not throw errors:

    suspend fun getExtensions(id: String?) : List<Extension>{
        return withContext(Dispatchers.IO) {
            extensionsService.getByIdAsUni(id)
        }.awaitSuspending().toList()
    }

see full example here

Expected response: expected-behavior

Actual behavior

Quarkus uses not the same class loader inside a coroutine scope as outside while working with generated rest clients.

outside classloader: Quarkus Runtime ClassLoader: DEV for rest-client-quickstart-1.0.0-SNAPSHOT restart no:0
inside classloader: Quarkus Base Runtime ClassLoader: DEV for rest-client-quickstart-1.0.0-SNAPSHOT

Error response: actual-behaviour

How to Reproduce?

The example below is based on these docs but translated to Kotlin.

  1. clone an example project git clone https://github.com/Sic4Parvis9Magna/quarkus-rest-client-issue.git
  2. run the project mvn compile quarkus:dev
  3. Use Postman collection from here
  4. execute Get extension co request and see the missing class in error response
  5. execute other requests to see rest client is working
  6. to fix the "broken" request you need to uncomment lines 20 & 22 here this fix was suggested in the original issue

Output of uname -a or ver

No response

Output of java -version

java version "21.0.1" 2023-10-17 LTS

Quarkus version or git rev

3.11.2

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

Apache Maven 3.9.6 (bc0240f3c744dd6b6ec2920b3cd08dcc295161ae)

Additional information

This issue is a follow-up on what was raised earlier for 3.9 as the solution is kinda unclear.

quarkus-bot[bot] commented 3 months ago

/cc @geoand (kotlin)

mschorsch commented 3 months ago

I dont understand why you wrap the Rest Client call into withContext? Quarkus REST Client supports suspend functions so you can simply mark your REST endpoints suspendable. Another option is to use https://smallrye.io/smallrye-mutiny/latest/guides/kotlin/ and use awaitSuspending on the returned Uni.

DarthRevanXX commented 3 months ago

@mschorsch due by using withContext(Dispatchers.IO) ensuring that such operations run on a thread pool optimized for I/O, preventing the main thread or other critical threads from being blocked. For example database calls, network calls

mschorsch commented 3 months ago

@DarthRevanXX The idea of reactive programming (Mutiny, Vert.x Futures/Promises, CompletionFuture, Kotlin Coroutines, ...) is simplified by being able to process many requests with just a few threads (Vert.x IO threads). Using a separate thread pool works against this idea and is not necessary and counterproductive in the case of the REST client.

DarthRevanXX commented 3 months ago

@mschorsch got it, makes sense, thanks

Sic4Parvis9Magna commented 3 months ago

@mschorsch , sorry for leaving this ticket w/o attention for a few weeks. So let's cross-check what we've got here to conclude the discussion and close the ticket.

  1. Even if the issue appears to be related to Quarkus changing class loaders in and out of the coroutine's scope with migration to 3.9 it is not what we are about to "fix" here as it works as intended.
  2. What we need to fix is how we use Quarkus rest clients with suspension functions. To do so we need to move from:
    suspend fun getExtensions(id: String?) : List<Extension> {
        return withContext(Dispatchers.IO) {
            extensionsService.getByIdAsUni(id)
        }.awaitSuspending().toList()
    }

    To this:

    suspend fun getExtensions(id: String?): List<Extension> {
        return extensionsService.getByIdAsUni(id)
            .awaitSuspending().toList()
    }

Please confirm if I got everything right :pray:

mschorsch commented 3 months ago

@Sic4Parvis9Magna

I have tested your reproducer but could not reproduce the error you reported. Everything worked as desired without explicitly setting the classloader.

Even if the issue appears to be related to Quarkus changing class loaders in and out of the coroutine's scope with migration to 3.9 it is not what we are about to "fix" here as it works as intended.

I'm not part of the Quarkus team so I can't speak for the Quarkus team but I guess so. @geoand ist that right?

What we need to fix is how we use Quarkus rest clients with suspension functions.

Yes, that is more efficient. Another option is to mark the methods directly as suspendable in the rest client.

@Path("/extensions")
@Named("ExtensionsService")
@RegisterRestClient(configKey = "extensions-api")
interface ExtensionsService {
    @GET
    suspend fun getById(@RestQuery id: String?): Set<Extension>
}

// ...

@ApplicationScoped
class CoService(
    @Named("ExtensionsService")
    @RestClient 
    private val extensionsService: ExtensionsService
) {

   suspend fun getExtensions(id: String?): List<Extension> {
        return extensionsService.getById(id).toList()
    }
}
geoand commented 3 months ago

I'm not part of the Quarkus team so I can't speak for the Quarkus team but I guess so. @geoand ist that right?

Seems correct to me

geoand commented 3 months ago

Yes, that is more efficient. Another option is to mark the methods directly as suspendable in the rest client.

+1