quarkusio / quarkus

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

Allow `@WithTransaction` to work with kotlin coroutines #34101

Open mihaipoenaru opened 1 year ago

mihaipoenaru commented 1 year ago

Description

I'm using the quarkus-hibernate-reactive-panache-kotlin extension in my work project. I can't migrate to quarkus 3 because I'm using a lot of coroutine/mutiny interoperability so that I don't drown in Uni chaining. Unfortunately, @Transactional doesn't work anymore (as explained in the docs), and should be replaced with @WithTransaction.

I tried to do the basic example below:

@ApplicationScoped
class RepService {

    //supported :-)
    @WithTransaction
    fun testUni(): Uni<ReproducerEntity> {
        return ReproducerEntity().apply { name = "asdf" }.persist()
    }

    //not supported :-(
    @WithTransaction
    suspend fun testSus(): ReproducerEntity {
        return ReproducerEntity().apply { name = "asdf" }.persist<ReproducerEntity>().awaitSuspending()
    }
}

When I tried to call the suspending endpoint I got java.lang.IllegalStateException: A method annotated with @WithTransaction must return io.smallrye.mutiny.Uni: java.lang.Object testSus(kotlin.coroutines.Continuation<? super io.smallrye.mutiny.Uni<com.example.ReproducerEntity>> $completion) declared on com.example.RepService

Now, I'm not going to make this issue as a bug, because I've searched a lot and nothing in the docs leads me to believe that this feature exists at all, and the only other issue mentioning this is this one https://github.com/quarkusio/quarkus/issues/25563 but it is referring to a deprecated annotation.

I'm not sure if this is planned to happen any time soon, but until then, is there any other way to side-step this besides using Unis all over the place?

Implementation ideas

No response

quarkus-bot[bot] commented 1 year ago

/cc @evanchooly (kotlin), @geoand (kotlin)

geoand commented 1 year ago

Seems reasonable. @evanchooly do you plan on implementing this?

upeter commented 1 year ago

Today I played for the first time with Quarkus. And I have to say, I like it! Nevertheless, I stumbled on this issue.

Full Coroutine support would make reactive programming so much simpler - including the adoption of reactive Quarkus for developers. So, I strongly pledge to fix this issue as soon as possible. Springboot does this out of the box...

Having said that, I came up with a possible solution/workaround for this problem. I created a demo project, including tests for reactive and coroutines, which proves that it works.

Since I do not know enough about the internals of Quarkus, I don't know whether this solution will work for production purposes, e.g. under load. I also don't have time to investigate that. On the contrary, I think there is a good chance that it might just work...

Check it out: https://github.com/upeter/quarkus-panache-kotlin-coroutines

The key is the following:

@OptIn(ExperimentalCoroutinesApi::class)
suspend fun <T> withTransaction(block: suspend () -> T): T = Panache.withTransaction {
       CoroutineScope(Dispatchers.Unconfined).async { block() }.asUni()
}.awaitSuspending()

Usage:

@POST
suspend fun create(fruit: Fruit): Response = withTransaction {
      fruit.persist<Fruit>().awaitSuspending().let { Response.ok(it).status(Response.Status.CREATED).build() }
}

For the details, check out my comment in the repo. Feedback is more than welcome!

geoand commented 1 year ago

@upeter that would come pretty close to a working solution, but it likely returns data on a wrong thread and also probably doesn't propagate the requests context correctly.

@evanchooly is this issue on your radar? I think it would be a good usability improvement.

mihaipoenaru commented 1 year ago

@upeter that looks neat, I was playing around with solutions as well, and I got close to what you proposed. Unfortunately, aside from what @geoand noticed, there is also the problem that I can't actually find the asUni() function. Maybe I'm missing something.

Anyway, I'm not sure how easy it would be to make an official solution, so I'm down for just a workaround. I'm not in a rush because our current project's migration is also stuck in https://github.com/quarkusio/quarkus/issues/34099, so I guess we'll stay a while in 2.16.7

geoand commented 1 year ago

I can't actually find the asUni() function. Maybe I'm missing something

My guess is that you are missing the proper dependency

mihaipoenaru commented 1 year ago

Oops, your guess would be correct 😆 . Still if there's some thread context management issues, I'd rather just wait, Thanks for your help!

upeter commented 1 year ago

@upeter that would come pretty close to a working solution, but it likely returns data on a wrong thread and also probably doesn't propagate the requests context correctly.

@evanchooly is this issue on your radar? I think it would be a good usability improvement.

After sleeping a night over, I came to the same conclusion. I already thought it was too good to be true :-) .

However, Coroutines support Context propagation, even with ThreadLocals via a ThreadContextElement implementation. There is a good chance that a Coroutine Context approach might work, though it requires good knowledge of the Quarkus internals, which - I assume - you @geoand definitely have.

Support for @WithTransaction is probably harder to provide, but a working withTransaction {...} code block could come in very handy in the meantime.

geoand commented 1 year ago

Support for @WithTransaction is probably harder to provide

Indeed, but not that much :)

polymorph233 commented 1 year ago

Is there any possibility to use suspend fun signature for methods annotated with @ConsumeEvent and listening to the vert.x event bus? Adding suspend will add a second parameter of type Continuation which breaks the dependency injection and in case of using Multimap for the first one, the second one is expected to be JsonObject therefore no way to use coroutines here.

mihaipoenaru commented 1 year ago

@geoand @evanchooly any idea if this issue is on the roadmap? We will soon (this year) have to migrate to Quarkus 3. If this won't be implemented, I need to know beforehand so that I can just bite the bullet and rewrite the panache logic to use Uni chaining.

geoand commented 1 year ago

@mihaipoenaru we'll take a look next week and determine how much work it requires

ValentineSokol commented 12 months ago

Any update here? Still running into the same issue. Would be nice to get coroutine support with this

ValentineSokol commented 12 months ago

My little workaround to maintain a Mutiny Session without @WithTransaction and still use coroutines:

object Valnache {
    private val sf = Arc.container().instance(Mutiny.SessionFactory::class.java).get()

    val sessions: MutableMap<String, Mutiny.Session> = mutableMapOf()

     suspend fun getSession() : Mutiny.Session {
         val threadName = Thread.currentThread().name
         println("Running on thread $threadName")
        val context: Context = Vertx.currentContext()

         val sessionKey = BaseKey(Mutiny.Session::class.java, (sf as Implementor).uuid)

        if (sessions.containsKey(threadName)) {
            println("Mutiny session found on thread $threadName, skipping init")
        } else {
            println("No session in context, initializing on thread $threadName")
            sessions[threadName] = sf.openSession().awaitSuspending()
            println("Session initialized ${sessions[threadName]?.isOpen}")
        }

        context.putLocal(sessionKey, sessions[threadName])
        return sessions[threadName]!!
    }

  suspend fun <T> query(work: () -> Uni<T>) : T {
      getSession()
      return work().awaitSuspending()
  }
  suspend fun <T> transaction(work: (Mutiny.Transaction) -> Uni<T>) : T {
        return getSession().withTransaction(work).awaitSuspending()
    }
}
val userRecord = Valnache.query { UsersRepository.find("username", username).firstResult() }.awaitSuspending()

`

TL;DR coroutines seem to lose a session from vertxContext possibly due to resuming on a different thread. So instead we inject a session before every request, which works.

devpikachu commented 10 months ago

@geoand Do you have any updates on this? /cc @evanchooly

geoand commented 10 months ago

Unfortunately I have zero time to work on this for the next couple months or so

mihaipoenaru commented 5 months ago

@geoand is this still on the radar to be done? We have a task to migrate to quarkus 3 this year, and it would be quite painful to do a workaround.

geoand commented 5 months ago

Unfortunately this is not a priority currently.

samy-ljn commented 5 months ago

I hope it will be soon 🙏

miladjafary commented 4 months ago

@geoand , is there any progress on this? I would say, it's an obstacle for everyone who would like to use hibernate-reactive + kotlin coroutine and if Quarkus does not support it, then we should scarify either kotlin coroutine or hibernate reactive which both are really cool feature. I would like to ask you do us a favor and consider it as a Quarkus priority.

geoand commented 4 months ago

I totally understand where you are coming from. However, there is no way I can personally make this a priority. All I can do is keep it on the backburner until some extra time presents itself. That of course does not mean that others can't take a shot at it - on the contrary, we very much welcome contributions

miladjafary commented 4 months ago

@geoand, I am not expert on the Quarkus framework implementation, however, If you could share any document and pave the way in that regards, then I might be able to contribute for this specific issue to unblock ourself.

geoand commented 4 months ago

The reference doc is https://quarkus.io/guides/writing-extensions

Beware however because it's a very dense read. It might make more sense to look at other Kotlin support functionality in the source code