Repository navigation
TimeLimiter should be optional #174
Description
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.
Activity
ryanjbaxter commentedon Oct 25, 2023
I think this makes sense, would you be interested in submitting a PR with these changes?
msiegemund commentedon Oct 26, 2023
Yes, i am interested.
ryanjbaxter commentedon Oct 26, 2023
Great! If you can make the changes and submit a PR we would welcome the contribution!
msiegemund commentedon Oct 28, 2023
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#disableThreadPooltofalse.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#runsimply invokes the provided supplier, which in turn is exactly what i intended to happen.Nevertheless, the
org.springframework.cloud.circuitbreaker.resilience4j.ReactiveResilience4JCircuitBreakerdoes 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:
spring.cloud.circuitbreaker.resilience4j.disable-time-timiterwhich then additionally removes the timelimit from the reactive implementation orIn 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.Or maybe even worse due to its logical incompatibility.
I am looking forward for some input in regard to this.
ryanjbaxter commentedon Oct 30, 2023
I think its ok to introduce the
disable-time-timiterproperty but it would only have effect in the non reactive case when using a thread pool.