Avoid using @ConditionalOnClass(Flux.class) and similar conditions

[vpavic]

The current Spring Boot codebase contains two instances of @ConditionalOnClass(Flux.class):

spring-boot/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/health/HealthIndicatorAutoConfiguration.java

Lines 83 to 85 in 7946bec

	@Configuration
	@ConditionalOnClass(Flux.class)
	static class ReactiveHealthIndicatorConfiguration {

spring-boot/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/web/reactive/ReactiveManagementContextAutoConfiguration.java

Lines 35 to 38 in 7946bec

	@Configuration
	@ConditionalOnClass(Flux.class)
	@ConditionalOnWebApplication(type = Type.REACTIVE)
	public class ReactiveManagementContextAutoConfiguration {

When a Servlet based app depends on a library that transitively pulls in Project Reactor as mandatory dependency, such as Lettuce, the first example will result in ReactiveHealthIndicatorRegistry bean registered, which I believe shouldn't be the case in a Servlet app.

The second example is benign and has no consequences, but arguably having @ConditionalOnClass(Flux.class) there is needless as it will get shadowed by @ConditionalOnWebApplication(type = Type.REACTIVE).

IMO the conditions that test the presence of Project Reactor on classpath should be avoided due to high likeliness of false positives, unless there's really a valid case for those.

[bclozel]

Unless I'm missing something, the first instance in ReactiveHealthIndicatorConfiguration is about setting up the reactive infrastructure for any reactive health indicator. I don't know if we can refine the condition here, but in the case of Lettuce, there might be a reactive health indicator to set up for that so we need the infrastructure ready.

Worst case scenario, the application only declares blocking HealthIndicator and those are executed on an elastic scheduler. Is there any other downside that I've missed?

Now about the second case; it's true that this @ConditionalOnClass might have false positives, but I think that having an auto-configuration class without any @CondiontalOnClass is way worse; classpath conditions are cheap and can prevent the class from being loaded if the condition fails. Even with possible false positives, removing that condition will penalize all other cases. The bottom line is: if there's any other safe class that we can use here, less prone to false positives, we should consider it. Do you have any other class in mind?

[vpavic]

For Servlet based app, I don't think the choice of Redis driver should influence whether reactive infrastructure should be configured. This is what happens now - if I use Jedis there's no ReactiveHealthIndicatorRegistry bean registered.

That's why I think that relying on the presence of Project Reactor on classpath isn't the right thing to do here, but rather to react to the fact that some of the Spring Data's components for reactive Redis have been configured.

I don't do much reactive so I don't know what would be the best thing to do here, but since core of Project Reactor is simply a foundation for building reactive apps and can be used in so many different scenarios, I don't think its mere presence on classpath should influence how infrastructure is set up.

[bclozel]

Indeed, using Jedis doesn't trigger this piece of infrastructure; but I'm not seeing any downside in enabling that actuator part in case reactive bits are present. Even if the health indicators are not exposed through a web interface, they'll still be executed in a more efficient way.

Setting up a condition on Spring Data doesn't really matter here, since the health indicator usually rely on low-level constructs/drivers. As you said, Reactor is a foundation for reactive libraries and it's the best signal we can pick up here IMO.

The team has discussed that point and we feel that attempting to change that might result in blocking reactive infrastructure in some cases, whereas right now worst thing that can happen is wrapping blocking infrastructure with non-blocking calls.