micronaut-projects / micronaut-core

Micronaut Application Framework
http://micronaut.io
Apache License 2.0
6k stars 1.04k forks source link

Threading model for HTTP Client and Java 21+ #10933

Open Nahuel92 opened 3 days ago

Nahuel92 commented 3 days ago

Issue description

Hi there!

I have a doubt about the threading model of the HTTP clients (low-level and declarative).

The HTTP low-level client docs (scroll down to Performing an HTTP GET) mention that non-blocking reactive types should be preferred over non-reactive ones:

Note that in this example, for illustration purposes we call toBlocking() to return a blocking version of the client. However, in production code you should not do this and instead rely on the non-blocking nature of the Micronaut HTTP server.

And same thing happens in declarative HTTP client docs (you'll need to scroll down to find it):

This is a non-blocking reactive type - typically you want your HTTP clients to not block. There are cases where you may want an HTTP client that does block (such as in unit tests), but this is rare.

I have been using the declarative HTTP client by default, and all my methods return non-reactive types (I return DTOs directly which, I assume, are blocking types).

I understand this is can impact performance and I would like to know if:

Another related question is about @CircuitBreaker. I couldn't find anything in the docs about how it works with HTTP 400 Bad Requests. What I can tell is that, to me, it doesn't make any sense to retry a Bad Request. This isn't documented and I would like to know if someone can clarify it, , but again, I'm not sure of the current behavior (I plan on creating a PoC, perhaps tomorrow). Here's a link to a PoC app I made that proves that @CircuitBreaker does retry (at least) 400 Bad Requests: https://github.com/Nahuel92/micronaut-circuitbreaker-poc

Thanks in advance!

yawkat commented 2 days ago

While reactive types are still faster, with virtual threads the main disadvantages disappear, so yes we should remove that recommendation.

Using blocking types will block the event loop thread (so that I go ahead and fix my HTTP clients).

Blocking operations can still block the event loop. But if you use ExecuteOn or micronaut.server.thread-selection=blocking, most of the code will not run on the event loop, so this is a non-issue.

Nahuel92 commented 2 days ago

While reactive types are still faster, with virtual threads the main disadvantages disappear, so yes we should remove that recommendation.

Using blocking types will block the event loop thread (so that I go ahead and fix my HTTP clients).

Blocking operations can still block the event loop. But if you use ExecuteOn or micronaut.server.thread-selection=blocking, most of the code will not run on the event loop, so this is a non-issue.

Hi @yawkat, thanks for answering.

Can you tell me if adding @ExecuteOn to my declarative client definition is enough to not block the event loop? Or should it be added to the non-private methods that use it instead?

yawkat commented 2 days ago

I think you are misunderstanding what "blocking the event loop" means. It does not matter how you do the blocking, whether the client is annotated with ExecuteOn (which I believe does nothing anyway), or even whether you block in some completely different way that does not involve BlockingHttpClient. What matters is that the controller must not block at all when it runs on the event loop. So if you use blocking operations like BlockingHttpClient, you must make sure that the controller is annotated with ExecuteOn (or the equivalent config property). Then what you do in the controller doesn't matter.

Nahuel92 commented 2 days ago

I think you are misunderstanding what "blocking the event loop" means. It does not matter how you do the blocking, whether the client is annotated with ExecuteOn (which I believe does nothing anyway), or even whether you block in some completely different way that does not involve BlockingHttpClient. What matters is that the controller must not block at all when it runs on the event loop. So if you use blocking operations like BlockingHttpClient, you must make sure that the controller is annotated with ExecuteOn (or the equivalent config property). Then what you do in the controller doesn't matter.

Got it. In the app I'm currently working on, I'm not using a Controller. It's a cron job that uses virtual threads to run a bunch of tasks (calling an external service via HTTP and processing the response in the same unit of work, basically).

From what you just shared; I believe I'm already covered, but feel free to correct me if I'm wrong. The only thing that I just did is to remove the @CircuitBreaker annotation because it was messing with my WireMock tests. That's a different discussion anyway (as I see it, Bad Requests shouldn't be retried).

Thanks a lot :)