RestClient Reactive: Context not propagated

[indalaterre]

Describe the bug

Hi. I'm working with a blocking implentation of Hibernate using the @Blocking annotation. As is the software is operating with the database without issues. Adding a http request in the chain using Restclient reactive than I have the following message that probabily indicates that the context is not propagated even if quarkus-smallrye-context-propagation is added.

2021-05-03 15:10:07,939 ERROR [org.jbo.res.rea.com.cor.AbstractResteasyReactiveContext] (executor-thread-198) Request failed: java.lang.IllegalStateException: You have attempted to perform a blocking operation on a IO thread. This is not allowed, as blocking the IO thread will cause major performance issues with your application. If you want to perform blocking EntityManager operations make sure you are doing it from a worker thread.

To Reproduce

Here a small reproducer
demo.zip

Inside you have a working and a non working API

Output of java -version

openjdk version "11.0.11" 2021-04-20

Quarkus version or git rev

Quarkus 1.13.3

[quarkus-bot]

/cc @michalszynkiewicz

[geoand]

This is an interesting use case.

What happens is that when the rest client resumes execution (i.e. an HTTP client response is created), execution resumes on a Vert.x event loop thread instead of a worker pool thread (as is appropriate because in this case the endpoint is a RESTEasy Reactive endpoint using @Blocking).

There are two things we can do:

1. Fix this case and indeed return onto the worker pool
2. Disallow this case altogether as it does not make sense a tremendous amount of sense to service the server request on a worker thread and not use that thread entirely for the duration of the rest call (the thread can't be used anyway until the server request is completed).

I believe the second option is the best.

@cescoffier @FroMage @michalszynkiewicz WDYT?

[cescoffier]

I would also go for 2 until we have lots of demand for this use case.

If you block the thread for the HTTP request (which makes sense you are on a worker thread anyway), it should work. If you REST client returns a Uni, use .await().atMost(Duration.ofSecond(somethingSensible))

[FroMage]

So first: this is not about context propagation, because I assume the context is properly propagated. If there was a @Transactional annotation it should also be propagated.

This is purely about ORM refusing to work on a vert.x IO thread, which is a Good Thing™.

What's peculiar in this use-case is that we're using @Blocking for no reason, because we're returning a Uni what will not actually run on the worker thread, and we'll suspend the request anyway, and return that worker thread anyway. So we're not actually blocking. Are you sure we don't return that worker thread to the pool when the endpoint method terminates, @geoand ? Do we reserve it for later handler steps?

I think we could document this, or perhaps improve the error message by telling users to use scheduleOn with the right thread pool. Not sure what the exact syntax is for that, @cescoffier ?

@geoand this sort of mistake we can detect in RR by catching the exception thrown by ORM and if we notice the endpoint was async (returned a Uni) we can turn it around into a message saying "you've resumed your Uni on an IO thread, but you chained an operation that is blocking, so add scheduleOn something something to fix it".

Or we could add that scheduleOn ourselves if we see @Blocking but I suspect it has to be called before the problematic map that calls ORM, right @cescoffier?

[geoand]

So first: this is not about context propagation, because I assume the context is properly propagated. If there was a @Transactional annotation it should also be propagated.

This is purely about ORM refusing to work on a vert.x IO thread, which is a Good Thing™.

What's peculiar in this use-case is that we're using @Blocking for no reason, because we're returning a Uni what will not actually run on the worker thread, and we'll suspend the request anyway, and return that worker thread anyway. So we're not actually blocking. Are you sure we don't return that worker thread to the pool when the endpoint method terminates, @geoand ? Do we reserve it for later handler steps?

We do return the thread to the pool, but my point is that doing this mixing and matching is very error prone and we should likely prevent it when we can.

I think we could document this, or perhaps improve the error message by telling users to use scheduleOn with the right thread pool. Not sure what the exact syntax is for that, @cescoffier ?

@geoand this sort of mistake we can detect in RR by catching the exception thrown by ORM and if we notice the endpoint was async (returned a Uni) we can turn it around into a message saying "you've resumed your Uni on an IO thread, but you chained an operation that is blocking, so add scheduleOn something something to fix it".

Or we could add that scheduleOn ourselves if we see @Blocking but I suspect it has to be called before the problematic map that calls ORM, right @cescoffier?

To be honest, I don't think that mixing scheduleOn is the way to go. It just confuses things even further.
IMHO, we should just disallow returning Uni when @Blocking is used.

[michalszynkiewicz]

out of curiosity, would that work with vertx.executeBlocking or would we lose the context?

[cescoffier]

@FroMage USing blocking calls is documented on the mutiny documentation: https://smallrye.io/smallrye-mutiny/guides/imperative-to-reactive.

The problem is that after you dispatched the subscription (runSubscriptionOn) or the event (emitOn), you don't go back to the previous thread, which means we need to be sure that everything is captured and restored.

[FroMage]

IMHO, we should just disallow returning Uni when @Blocking is used.

Huh, that's interesting. I wonder if there's any legitimate use-case for allowing it.

[geoand]

IMHO, we should just disallow returning Uni when @Blocking is used.

Huh, that's interesting. I wonder if there's any legitimate use-case for allowing it.

From what I have seen, users do that because either they don't understand what is going on under the hood or because they want to use some of the API that Uni provides

[indalaterre]

If I can give my experience I use the reactive APIs with Blocking annotation because I'm still use blocking Hibernate waiting for the stability of the reactive one. This helps me with the migration as I wouldn't refactor the entire application

[geoand]

But in that case why would you use coroutines? Why not just use blocking APIs for the entire request handling?

[indalaterre]

Because I already have the reactive code in place

[geoand]

Is that code actually non blocking though or does it just use reactive types?

[indalaterre]

It's completely non blocking except Hibernate that is executed within a deferred Uni

[geoand]

Personally I don't see the point in mixing that way, but I understand where you are coming from.

The problem is it can lead to unexpected behavior.

[FroMage]

Can you show us your code? If Hibernate is executed in a deferred Uni without a specified scheduler, it can lead to unexpected results, depending on who starts waiting for its value, so @Blocking might actually create a false sense of what's happening even in your case.

[indalaterre]

Hi this is the main repository class that I'm using right now
repo.txt

[geoand]

Can you retry this with 2.3.0.Final? If it continues to not work, can you attach a reproducer project please?

[geoand]

Closing this as there were some relevant fixes that have gone in.
If it's still an issue, please comment or reopen