TimeLimiter should be optional

[msiegemund]

now

Currently it is not possible to deactivate the TimeLimiter when using the circuitbreaker via org.springframework.cloud.client.circuitbreaker.CircuitBreakerFactory. The org.springframework.cloud.circuitbreaker.resilience4j.Resilience4JAutoConfiguration#resilience4jCircuitBreakerFactory defaults to a TimeLimiterRegistry which in turn always provides the default io.github.resilience4j.timelimiter.TimeLimiterConfig#timeoutDuration of 1s.

Later on, the org.springframework.cloud.circuitbreaker.resilience4j.Resilience4JCircuitBreaker#run relies on the presence of a TimeLimiter since io.github.resilience4j.timelimiter.TimeLimiter#decorateFutureSupplier(io.github.resilience4j.timelimiter.TimeLimiter, java.util.function.Supplier<F>) is used to wrapp the invocation.

So there is no out-of-the-box-way to deactivate the TimeLimiter functionality of the spring-cloud-circuitbreaker via .yaml. If you want to use the circuitbreaker you get stuck with the TimeLimiter as well.

If there is already a proper way to use the spring-cloud circuitbreaker by making use of the CircuitBreakerFactory without the TimeLimiter, this feature request can be dropped immediately and no further reading is required. ;)

why

As an example, in a use case where a socket/port (with provided timeouts) gets opend within the circuitbreakers invocation, there is no need for a TimeLimiter on a higher level. Additionally it can lead to serious resource leaks on certain operating systems to interrupt the calling Thread if a blocking I/O operation is running within the circuitbreakers invocation and somebody forgot to define a timeout on the I/O operation itself. Within this scenario the Thread could be interrupted but the I/O operation stays open/blocked, representing a possibly undetected resource leak.

workaround

As a workaround, we currently override the spring-cloud TimeLimiter with our own one which uses a timeout higher than the one used by the inner operation.

/**
 * This configuration provides a custom {@link TimeLimiterRegistry}, used by <code>spring-cloud-circuitbreaker-resilience4j</code>.
 * This configuration loaded conditionally, if <code>override-spring-cloud-timelimiter/code> is set to <code>true</code>.
 */
@Configuration
@ConditionalOnProperty(name = "override-spring-cloud-timelimiter")
public class SpringCloudResilience4JTimeLimiterConfig {

    private static final Logger LOG = LoggerFactory.getLogger(SpringCloudResilience4JTimeLimiterConfig.class);

    /**
     * Retrieve a {@link TimeLimiterRegistry} which overrides the default timeout
     * by using the socket's read timeout and multiplies it by two.
     * <p>
     * This is necessary since the sprint-cloud-resilience4j implementation does not permit using the circuit-breaker
     * without using a time-limiter.
     *
     * @param timeoutMs the read timeout in milliseconds
     * @return the customized {@link TimeLimiterRegistry}
     * @see org.springframework.cloud.circuitbreaker.resilience4j.Resilience4JAutoConfiguration
     * @see org.springframework.cloud.circuitbreaker.resilience4j.Resilience4JCircuitBreaker#run(Supplier, Function)
     */
    @Bean
    @Primary
    public TimeLimiterRegistry timeLimiterRegistry(@Value("${resttemplate.readtimeout}") int timeoutMs) {
        LOG.info("overriding spring-cloud circuitbreaker TimeLimiterRegistry");
        return new InMemoryTimeLimiterRegistry(
                TimeLimiterConfig.custom()
                        .timeoutDuration(Duration.ofMillis(timeoutMs * 2L))
                        .cancelRunningFuture(false)
                        .build()
        );
    }
}

desired

It would be beautiful to either have a property provided which in turn would deactivate the TimeLimiter explicitely. Otherwise it would be most logical to not activate/use the TimeLimiter within the Resilience4JCircuitBreaker if no TimeLimiter configuration is provided. But as a drawback, this would be a semantic API break which is probably not desired.

[ryanjbaxter]

I think this makes sense, would you be interested in submitting a PR with these changes?

[msiegemund]

Yes, i am interested.

[ryanjbaxter]

Great! If you can make the changes and submit a PR we would welcome the contribution!

[msiegemund]

With some deeper insights, i just realized that it is already possible to logically disable the use of the TimeLimiter (within the scope of the non-reactive implementation) by setting the property org.springframework.cloud.circuitbreaker.resilience4j.Resilience4JConfigurationProperties#disableThreadPool to false.

This property is referenced/checked within org.springframework.cloud.circuitbreaker.resilience4j.Resilience4JCircuitBreakerFactory#create(java.lang.String, java.lang.String, java.util.concurrent.ExecutorService). When disabling the tread pool, org.springframework.cloud.circuitbreaker.resilience4j.Resilience4JCircuitBreaker#run simply invokes the provided supplier, which in turn is exactly what i intended to happen.

Nevertheless, the org.springframework.cloud.circuitbreaker.resilience4j.ReactiveResilience4JCircuitBreaker does not benefit from this implicit logical behavior. Within the reactive implementation, the timeout is always set (.timeout(tuple.getT2().getTimeLimiterConfig().getTimeoutDuration()))

These observations lead to the following question and possibilities:

• Is it desired to introduce a new property/parameter (e.g. spring.cloud.circuitbreaker.resilience4j.disable-time-timiter which then additionally removes the timelimit from the reactive implementation or
• is it not desired and i will not continue in doing so?

In any circumstances, it is probably desirable to document this already complex behavioral configuration/decision tree somehow. Introducing a new disable-time-timiter-Property would lead to an even more complex configuration behavior. When introduced, the implementation whould need to deal with the following combination of properties.

spring.cloud.circuitbreaker.resilience4j.disable-thread-pool=false
spring.cloud.circuitbreaker.resilience4j.disable-time-timiter=true

Or maybe even worse due to its logical incompatibility.

spring.cloud.circuitbreaker.resilience4j.disable-thread-pool=true
spring.cloud.circuitbreaker.resilience4j.disable-time-timiter=false

I am looking forward for some input in regard to this.

[ryanjbaxter]

I think its ok to introduce the disable-time-timiter property but it would only have effect in the non reactive case when using a thread pool.