Andy Wilkinson opened SPR-16643 and commented

Spring Boot's new Actuator endpoint model allows users to write endpoints with operations that may block. We need to adapt those operations to both WebMVC and WebFlux. When adapting to WebFlux, we identify operations that may block and use publishOn so that the work is performed on a different thread. We currently use the operation's return type to determine whether or not it will block, erring on the side of assuming that it will.

If Framework provided a mechanism for indicating whether or not a request mapping would block (an @Blocking annotation has been discussed) and automatically did the publishOn, we could adopt this approach in Spring Boot's Actuator and provide a consistent experience across Framework and Boot.

Affects: 5.0.4

Comment From: spring-projects-issues

Rossen Stoyanchev commented

Thanks Andy Wilkinson, for starting the discussion with a concrete use case. Given the default non-blocking assumptions in WebFlux, now also documented #21081, some kind of opt-out mechanism is logical.

My first thought was to see an @Blocking annotation as a natural convenience vs directly using the publishOn operator of Reactor or RxJava within the controller method, but, come to think of it, it may not always have the intended result. For example, in the below scenario automatically switching threads at the point of invoking the controller method is not the same as switching explicitly inside it:

@GetMapping(/foo')
public Mono<Void> handle(@RequestBody Mono<Foo> foo) {
    return foo.flatMap(...) // some other async operation (e.g. remote call) here
            .publishOn(...)
            .map(...) // blocking call here
            .then();
}

The second point is to have a way to specify what thread pool to use (e.g. parallel, elastic). Currently under the covers WebFlux would have to use a Reactor Scheduler since we don't have any abstraction for switching threads with different reactive libraries. So if the application is using RxJava that is not the ideal solution.

As far as I can see, the main target for such an annotation would be controller methods that don't use reactive types at all. Or otherwise, it seems more natural and flexible to use publishOn explicitly in the code. We'd have to explain and position such an annotation in those terms.

Thoughts?

/cc Sébastien Deleuze

Comment From: spring-projects-issues

Andy Wilkinson commented

I agree that automatically switching threads when invoking the controller would not do the right thing. The problem in Boot's Actuator is more constrained. We don't expect an endpoint to be using any reactive types so it's really just a single, potentially blocking call that we care about. Actuator endpoints are not performance critical as they're called relatively infrequently so we have the luxury of being able to dispatch to a separate thread when it may not strictly be necessary without it causing a problem.

As far as I can see, the main target for such an annotation would be controller methods that don't use reactive types at all

That makes sense to me and is a good summary of the Actuator endpoints where we had this problem.

Comment From: spring-projects-issues

Rossen Stoyanchev commented

As a point of interest, Ratpack has @NonBlocking and from examples in the codebase it is used on methods that return simple values, such as void, but are nevertheless asynchronous. There is also an @Blocks annotation but that doesn't seem to be used much.

It's worth mentioning this discussion is related to @Async, which has the opposite role, i.e. to mark a method as async within an imperative programming model, but the end result is the same, i.e. invoking the method on a different thread. I'm bringing this up because because if nothing else, @Async + a potential @Blocking will need to co-exist on some level, and one has to learn what each is for, and in what context, which can get confusing..

All this makes me reconsider the idea of having an annotation with such a broadly applicable name, but currently very limited scope (for controller methods). Perhaps a Blocking marker interface could mark all controller methods except the ones returning async types, along with the use of an elastic scheduler.

Comment From: spring-projects-issues

Rossen Stoyanchev commented

Putting this in the 5.0 backlog to allow more time for discussion and use cases.

Comment From: spring-projects-issues

Sébastien Deleuze commented

The fact that the main target is indeed for controller methods that don't use reactive type at all + the discussion about @Async makes me currently think that relying on that information to determine the thread to use to run such handler method is maybe the best way to address this need.

The behavior could be the following: - We use ReactiveAdapterRegistry#getAdapter to know if @RequestBody / @ResponseBody are both blocking (no adapter) - If both are blocking, we do automatically the publishOn - We add support for @Async in WebFlux to specify we don't want the publishOn behavior with blocking @RequestBody / @ResponseBody maybe with a new attribute to avoid confusion with Spring MVC behavior. - We also leverages Async#value which is documented as a qualifier value for the specified asynchronous operation(s). May be used to determine the target executor to be used when executing this method, matching the qualifier value (or the bean name) of a specific to specify with a convention to be determine the scheduler/executor to be used to execute an async/reactive HTTP exchange handling.

That seems to be more consistent than introducing a new @Blocking annotation that could lead to inconsistencies with @Async as pointed out by Rossen Stoyanchev.

Regarding to use cases and thinking about all the questions we had about mixing reactive (WebClient / Spring Data Reactive) and blocking (JPA, legacy library implying IO) in the same webapp, I think there is a strong need for supporting this. It could be argued that such people could use Spring MVC, but you then loose WebFlux benefits in term of processing (back pressure, better streaming support, Netty based stack, etc.) but also you just can't the functional programming model on server-side.

Also I see this as a nice complement to Reactor non-blocking thread protection (which ideally would have been a Reactor Core 3.2 / Spring Framework 5.1 feature IMO) to provide more guidance and control for the threading model in addition to the great new documentation introduced as part of #21081.

Any thoughts?

Comment From: spring-projects-issues

Sébastien Deleuze commented

After discussing that during the Reactive meeting, please let me update my proposal.

My main point is that currently, it is quite easy to block the few Netty event loop threads using blocking calls, and IMO we should be more defensive about that.

Like we support applications that are 80% blocking and 20% Reactive in Spring MVC, the goal here would be to support applications that are 80% Reactive and 20% blocking out of the box in WebFlux with sane defaults. While a non Reactive/Async return value is not a 100% sure way to determine if a blocking call has been performed, given the vast amount of such API available on the JVM using in the context of a @ResponseBody return value (blocking persistence, http client), I tend to think it is good enough to use that as an indication that by default we should isolate that in a dedicated scheduler/thread pool. Also this improves consistency between Spring MVC and Spring WebFlux. For example if I have an application that is using Spring Data MongoDB and Spring Data JPA, I will be able to migrate to Spring Data Reactive MongoDB by using Mono and Flux return values and keep using regular Pojo and List<Pojo> for JPA.

Based on our discussion, I agree that leveraging @Async is maybe not a good idea. This annotation is usually not that well understood in Spring MVC, so extending its usage to WebFlux would maybe cause more harm than good. If we don't to introduce new annotation, maybe just documenting that Mono.just() should be used to create in memory object without creating a new thread would be good enough. Introducing a dedicated @ResponseBody boolean parameter is maybe another option (not sure what name would be relevant and if we could find something that makes sense in Spring MVC world as well).

Comment From: rstoyanchev

Team Attention: with the team partially present, the following more current points came up.

After some time has passed, we now have R2DBC progressing, and removing a major reason for blocking methods. Project Loom is also likely to change calculations by making it less problematic to mix blocking and non-blocking. On the other hand Loom might also make it less of an issue to automatically switch threads for controller methods with a blocking signature.

We also have Blockhound to help detect issues with unintended mixing of blocking and non-blocking so we don't need to be as defensive.

For the time being we stick to the current behavior. One thing that we can do is provide some dedicated support to take a snippet of blocking code and run it off the Reactor thread so it's as easy as providing a lambda (e.g. not even having to call publishOn). This would also make it possible to do the same consistently in both annotated controllers and functional handlers, since otherwise there would be no answer for this in WebFlux.fn. To be decided if that belongs in Reactor or in the Spring Framework yet.

Comment From: rstoyanchev

Closing for now as there is no intention to do this but can be re-opened.

Comment From: rstoyanchev

This is now scheduled for 6.1 M2 in #30678.