Resilience4J Bulkheads - Thread doesn't get interrupted with Timelimiter & Bulkhead

Mistborn94 commented 1 year ago

Describe the bug When using a timelimiter + semaphore bulkhead, if the timelimiter's time is exceeded, the underlying thread executing is not interrupted but the bulkhead permit is released. This means if the restricted section inside the bulkhead is consistently timing out, more concurrent requests are submitted than what the bulkhead should allow.

This happens because the Resilience4JBulkheadProvider uses CompletableFuture.supplyAsync and Completable Futures can't be interrupted.

From the CompletableFuture.cancel docs:

Parameters: mayInterruptIfRunning - this value has no effect in this implementation because interrupts are not used to control processing.

Additional Article on CompletableFuture and interrupts: https://nurkiewicz.com/2015/03/completablefuture-cant-be-interrupted.html

Affected Versions 2.x and 3.x

Sample Sample repository: https://github.com/Mistborn94/spring-r4j-bulkhead-demo

The sample repository contains two test classes that are almost identical: BulkheadDisabledTests and BulkheadEnabledTests. BulkheadDisabledTests passes but BulkheadEnabledTests fails because the thread is not interrupted.

Possible solution

Use Bulkhead.decorateSupplier instead of Bulkhead.decorateCompletionStage to create the bulkhead and then use an executor service with a normal future (possibly the same one that is already used in the Resilience4JCircuitBreaker ) for the time limiter.
Swap the order of the bulkhead and the timelimiter so the time limiter happens inside the bulkhead and can interrupt the task (This would also fix the thread pool bulkhead which works only with CompletionStage and has a similar problem)

I would be willing to submit a pull request for this

ryanjbaxter commented 1 year ago

@Mistborn94 a PR would be quite welcome!

wllianwd commented 1 year ago

Not su if it is related but currently, the ThreadPoolBulkhead is not auto closing resources.

Tested by using Spring Cloud Circuit Breaker in a batch job, the batch hangs and never finishes as the bulkhead threads are not closed. When configure to use semaphore works fine.

More info here: https://github.com/resilience4j/resilience4j/issues/1199

Mistborn94 commented 7 months ago

Pull request has been up for a while now https://github.com/spring-cloud/spring-cloud-circuitbreaker/pull/164

spring-cloud / spring-cloud-circuitbreaker

Resilience4J Bulkheads - Thread doesn't get interrupted with Timelimiter & Bulkhead #163