Reactive REST Client breaks Hibernate Reactive Transactions

[markusdlugi]

Describe the bug

When combining the Reactive REST client with Hibernate Reactive, the Reactive REST client seems to break Context Propagation, leading to Hibernate Transactions not working as intended.

Expected behavior

Using Reactive REST Client does not have any negative effects on Hibernate Reactive

Actual behavior

Using Reactive REST Client will break Hibernate Reactive Transactions because a call to sessionFactory.withSession() within an existing transaction will result in a new session being created instead of re-using the previously existing session residing in context.

How to Reproduce?

Reproducer: https://github.com/markusdlugi/hibernate-reactive-with-transaction (don't get confused by the naming)

Steps to reproduce the behavior:

1. Run the application
2. Open http://localhost:8080/test. It will fail with java.lang.IllegalStateException: Couldn't find author 1
3. Remove the REST call (TestResource line 48). If you repeat the test, it will now work, you get an author and some books.
4. Revert your change and add the REST call again. The test will fail again.
5. Remove the Hibernate Transaction by replacing .withTransaction((session, tx) in line 43 with .withSession(session. The test will now work again, even though the REST call is still in place.

It seems like after the REST call, the session2 in the reproducer is not the same session as session1 where the entity was initially persisted. This leads to the error. Seems like the Reactive REST client messes up the context propagation.

Not sure why the example works when getting rid of the transaction entirely, though.

Output of uname -a or ver

Microsoft Windows [Version 10.0.18363.1556]

Output of java -version

OpenJDK 64-Bit Server VM Corretto-11.0.10.9.1 (build 11.0.10+9-LTS, mixed mode)

GraalVM version (if different from Java)

N/A

Quarkus version or git rev

2.0.3.Final

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

Apache Maven 3.6.3

Additional information

Possibly related to #16949? But this one seems more severe because you cannot mix Hibernate Reactive Transactions and the REST client at all.

[quarkus-bot]

/cc @DavideD, @Sanne, @gavinking, @michalszynkiewicz

[Sanne]

The contract of 'sessionFactory.withSession() is to open a new session: the Hibernate API does not make assumptions about scopes or contexts, operations are explicit. I guess we could make this clearer?

…

On Fri, 23 Jul 2021, 21:47 quarkus-bot[bot], ***@***.***> wrote: /cc @DavideD <https://github.com/DavideD>, @Sanne <https://github.com/Sanne>, @gavinking <https://github.com/gavinking>, @michalszynkiewicz <https://github.com/michalszynkiewicz> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18977 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKMTLFLENKTKLPHLKP6O3TZHINJANCNFSM5A4VMWKQ> .

[gavinking]

WDYM, @Sanne?

withSession() will return the existing session associated with the current Vert.x context (if there is one).

But I'm not sure why calling the rest client would change the Vert.x context.

[markusdlugi]

Yeah that's basically the question @gavinking. After the REST call returns the execution will resume on another event loop thread, but the session used to persist the entity is not in the context anymore.

After a good night's sleep I can also answer my own question:

Not sure why the example works when getting rid of the transaction entirely, though.

When there is no transaction, the session.persist(author) call will immediately result in the entity being written to the database, in which case we have no problem retrieving it later even though the session is a new one. In the case of a transaction though, we need to get exactly the same session because otherwise we won't be able to access the uncommitted work of the transaction.

[gavinking]

After the REST call returns the execution will resume on another event loop thread, but the session used to persist the entity is not in the context anymore.

OK so perhaps there's some way to avoid that. TBH it looks wrong to me, it's not what happens with the Vert.x SQL client, so I don't see why it should happen with the REST client (but surely I'm missing some subtleties).

[markusdlugi]

@michalszynkiewicz @geoand @FroMage Any idea why this would happen?

[geoand]

Does the problem manifest in 2.1.0.Final as well?

[markusdlugi]

Just updated the reproducer and tested it with that version again. It does also exist in 2.1.0.Final.

[geoand]

I'll try and have a look soon

[geoand]

The problem here stems from the fact that when the response from the rest client comes back, it comes back on a different event-loop thread than the original one (essentially in RESTEasy Reactive when the result in ready, we resume processing by submitting to the event loop here - with executor created here).

I thought that Vert.x had thread affinity so something like this could not happen. Apparently it's not the case
cc @FroMage @stuartwdouglas

[stuartwdouglas]

The vert.x io.vertx.core.net.impl.pool.SimpleConnectionPool does not appear to be maintaining affinity.

[geoand]

So I assume we need some input from the Vertx folks?

[geoand]

The vert.x io.vertx.core.net.impl.pool.SimpleConnectionPool does not appear to be maintaining affinity.

cc @cescoffier

[Sanne]

The problem here stems from the fact that when the response from the rest client comes back, it comes back on a different event-loop thread than the original one

We've observed this in HR as well, and that's the reason behind additional design changes and assertions we have now. You should not make any assumptions regarding execturing thread but stick with using the vertx context.

What is Reactive Resteasy doing which assumes "same thread" execution?

[geoand]

What is Reactive Resteasy doing which assumes "same thread" execution?

Nothing. It's Hibernate Reactive that fails in this case because when the rest client comes back it's on a different Vert.x thread.
RESTEasy Reactive does not assume anything and it doesn't care which thread the result comes back on

[Sanne]

It's Hibernate Reactive that fails in this case because when the rest client comes back it's on a different Vert.x thread.

oh, now I get it. Then it's probably my own fault, I'll need to review our internal assertions. Thanks, I'll check!

[Sanne]

@geoand sorry I'm confused - when you commented that I thought there was some trouble caused by my internal checks in org.hibernate.reactive.common.InternalStateAssertions#assertCurrentThreadMatches - but that doesn't seem to be happening with Markus's reproducer. Are you having other changes that got you to this conclusion?

All I see is that the context is lost when it's run on a different thread - I could go on and try to debug that but I'm puzzled that it's different than what you see?

[geoand]

@geoand sorry I'm confused - when you commented that I thought there was some trouble caused by my internal checks in org.hibernate.reactive.common.InternalStateAssertions#assertCurrentThreadMatches - but that doesn't seem to be happening with Markus's reproducer. Are you having other changes that got you to this conclusion?

Nope, I didn't change anything.

All I see is that the context is lost when it's run on a different thread - I could go on and try to debug that but I'm puzzled that it's different than what you see?

All I did to see that the result was coming back on another thread was add a couple break points and print the current thread.

When the rest-client was not being used, everything worked as expected because everything was being done on the same vert.x thread. However, when the rest client was added to the pipeline of operations (which it is in the reproducer), then once the result from that rest client call comes back, the rest of the pipeline seems to execute on another vert.x thread which I guess makes things fail. I have no knoweldge of what that is :)

[Sanne]

@geoand ok thanks for clarifying, that matches the same as what @DavideD and myself are seeing in debug.

Ok now I understand what Stuart said about affinity. Yes we will need that http client which is invoked to not have the context being lost. This doesn't seem related to Hibernate Reactive and I'm not familiar with these integrations.. who can own this?

[geoand]

I always took it for granted that Vert.x had thread affinity, but that doesn't seem to be the case. I assume we would need input from the Vert.x folks on this one.

[stuartwdouglas]

Yea, I think this needs input from the Vert.x guys. I think it is a bug in SimpleConnectionPool but I am not 100% sure what the expected semantics are.

[gavinking]

Vert.x does have thread affinity in at least some cases (for the SQL client it does, assuming I recall correctly what @vietj told me) but HR doesn't depend on it.

All we require is that the Vert.x context is preserved.

[FroMage]

OK, so is the Vert.x context preserved then?

[FroMage]

I assume not, otherwise this would work. So it's not "just" a thread affinity issue. Although I'm pretty sure we also need thread affinity to not get two parallel executions of the same context.

[FroMage]

I've passed on the question to the Vert.x team on discord. Pretty sure @cescoffier is on PTO: https://discord.com/channels/751380286071242794/751398938459766895/872058623394738236

[Sanne]

OK, so is the Vert.x context preserved then?

It isn't.

We have a context created and associated to the first thread, but after the REST client is invoked, subsequent operations are executed on a different thread and the original context is gone.
Vertx automatically creates a new context, but it's empty and that's causing the bug.

[FroMage]

Actually, we get notified from the client on a VertxThread which has a non-null context, and then from that thread, we grab a random EventLoop thread instead of using the one we are on https://github.com/quarkusio/quarkus/blob/2.1.0.Final/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/RestClientRequestContext.java#L205 (remember there are one event loop thread per CPU) and execute the callback on that thread, which does not have a context.
So it looks like our bug after all.

[geoand]

@FroMage thanks for that analysis. Are you going to take care of the issue?

[FroMage]

I probably can.

[geoand]

Thanks!

[FroMage]

#19225