Bulkhead - not working as expected

[scalablepuppets]

Yaml Config:

instances:
defaultInstance:
maxConcurrentCalls: 1
maxWaitDuration: 1ms

Java Code:
Method annotated with :
@bulkhead(name = "defaultInstance",fallbackMethod="fallback_BH")

private String fallback_BH(BulkheadFullException e) {
return "Bulkhead Full - fallback called";
}

Q- I am expecting that rest of the concurrent calls will be directed to fallback method once the limit of 1 call is reached on a method, but that's not happening , the calls are blocked on the method annotated with @bulkhead till it responds back.

Please suggest, if I need to change my implementation.

[RobWin]

Is that the full yaml file? The prefix resilience4j.bulkhead is missing.

[RobWin]

You are using a semaphore-based Bulkhead. Yes, it's normal that all threads are waiting until maxWaitDuration is elapsed before the exception is thrown.

[scalablepuppets]

Thanks Rob, please find the yaml config as below which am using, I was hoping that the exception would be thrown but that's not happening.

resilience4j.bulkhead:
configs:
default:
eventConsumerBufferSize: 10
maxConcurrentCalls: 1
instances:
defaultInstance:
maxConcurrentCalls: 1
maxWaitDuration: 1ms

[RobWin]

I can't reproduce it in our Spring Boot 2 demo.

I added locally a Thread.sleep(10000) (10s) to the BackendAConnector.success method and configured the Bulkhead as follows:

resilience4j.bulkhead:
    instances:
        backendA:
            maxConcurrentCalls: 1
            maxWaitDuration: 1s

I invoked the endpoint http://localhost:9080/backendA/success twice. The second call fails with

{
  "timestamp": "2019-08-29T06:44:29.563+0000",
  "path": "/backendA/success",
  "status": 500,
  "error": "Internal Server Error",
  "message": "Bulkhead 'backendA' is full and does not permit further calls"
}

When I add a fallback -> @Bulkhead(name = "backendA", fallbackMethod = "fallback")
The second call returns:

Recovered : io.github.resilience4j.bulkhead.BulkheadFullException: Bulkhead 'backendA' is full and does not permit further calls

[scalablepuppets]

really appreciate your time Rob ! I will test it shortly and update you.

[scalablepuppets]

Thanks, am able to produce it, just thought to share my observation as I see some scope of improvement:
a. with @bulkhead annotation, I can not specify the fallback method name, if I specify I get 'java.lang.NoSuchMethodException:fallback(,class java.lang.Throwable)' . error.

b. I used Postman Collection runner to send couple of concurrent request, in this case all the request went into the blocking mode and eventually got completed after Thread.sleep time was over.

c. If I initiate the request sequentially one after another (there is some lag time in switching) I get the desired results.

[RobWin]

Can you provide a GitHub repo where you reproduced it?

[RobWin]

How do you run concurrent requests in the Postman Collection Runner?
postmanlabs/postman-app-support#4198

[RobWin]

Even when I run multiple Collection Runners in parallel, it works for me.
When 1 call is running, the other calls are rejected after 10ms and the fallback returns a result.

[scalablepuppets]

Thanks Rob, I will test it tonight with another Rest Client.

[scalablepuppets]

Wrote a custom Python Script and tested using the trusted SOAPUI and it works as expected.

[cgarman1]

I'm actually seeing what I think is the same behavior with @bulkhead(name = "BACKEND", fallbackMethod = "fallback", type = Bulkhead.Type.THREADPOOL) where the fallback is completely ignored. However if I switch the type to Bulkhead.Type.SEMAPHORE everything works as I expected it should by calling the fallback method.

[RobWin]

Its fixed in the next version. Hopefully released tomorrow

[cgarman1]

Ok great! I will test once it is released.

[RobWin]

v.1.3.0 has been released. Maven Central Sync might take some time.

[cgarman1]

Didn't notice this before but it looks like both 1.2.0 and 1.3.0 still use a couple of the 1.1.0 modules...which now produces a boot startup error with 1.3.0

An attempt was made to call a method that does not exist. The attempt was made from the following location:
io.github.resilience4j.circuitbreaker.configure.CircuitBreakerConfiguration.createCircuitBreakerRegistry(CircuitBreakerConfiguration.java:141)
The following method did not exist:
io.github.resilience4j.circuitbreaker.CircuitBreakerRegistry.of(Ljava/util/Map;Lio/github/resilience4j/core/registry/RegistryEventConsumer;Lio/vavr/collection/Map;)Lio/github/resilience4j/circuitbreaker/CircuitBreakerRegistry;

[RobWin]

The Spring Boot 2 demo works fine. Sure it's not an issue in your build script?

Which dependencies do you use?

[cgarman1]

I don't think so because the only import I have in my pom is resilience4j-spring-boot2...maven is bringing in everything else. Otherwise I have removed all application config except annotations...but I'm checking

    <dependency>
      <groupId>io.github.resilience4j</groupId>
      <artifactId>resilience4j-spring-boot2</artifactId>
      <version>1.3.0</version>
    </dependency>

[cgarman1]

Once I added the following dependencies to override what is coming in by default it starts up fine...see below.

However, I'm still seeing the same behavior I commented on yesterday where the fallback method of @bulkhead doesn't fire when type = Bulkhead.Type.THREADPOOL and a io.github.resilience4j.bulkhead.BulkheadFullException exception is instead thrown back to the client. I was expecting to be able to catch and handle this situation myself in the fallback method.

    <dependency>
      <groupId>io.github.resilience4j</groupId>
      <artifactId>resilience4j-circuitbreaker</artifactId>
      <version>1.3.0</version>
    </dependency>
    <dependency>
      <groupId>io.github.resilience4j</groupId>
      <artifactId>resilience4j-micrometer</artifactId>
      <version>1.3.0</version>
    </dependency>
    <dependency>
      <groupId>io.github.resilience4j</groupId>
      <artifactId>resilience4j-timelimiter</artifactId>
      <version>1.3.0</version>
    </dependency>

[RobWin]

Can you show me how your code looks like?

[cgarman1]

Ok I had to sanitize things a bit but this is what I'm doing...I am expecting to see the exception get logged and not make is back to the caller.

	@Bulkhead(name = "BACKEND", fallbackMethod = "retrieveAccountDetailsFallback", type = Bulkhead.Type.THREADPOOL)
	@Override
	public CompletableFuture<ResponseTO> retrieveAccountDetails(AccountDetailsRequestTO request) {
		try {
			return CompletableFuture.completedFuture(client.getAccountDetails(request));
		} catch (RuntimeException e) {
			LOGGER.error(NestedExceptionUtils.getMostSpecificCause(e).getMessage());
			return retrieveErrorResponse();
		} 
	}
	
	@Override
	public CompletableFuture<ResponseTO> retrieveAccountDetailsFallback(AccountDetailsRequestTO request, Exception e) {
		LOGGER.error(NestedExceptionUtils.getMostSpecificCause(e).getMessage());
		return retrieveErrorResponse();
	}

[RobWin]

I investigate it tomorrow in more depth. I come back to you. Maybe it's an issue in the BulkheadAspect error handling. If you also use add a CircuitBreaker (like in the demo) and add your fallback method there, it should definitely work.

[cgarman1]

Yes, it definitely works when I add the @CIRCUITBREAKER

[dlsrb6342]

Didn't notice this before but it looks like both 1.2.0 and 1.3.0 still use a couple of the 1.1.0 modules...which now produces a boot startup error with 1.3.0

An attempt was made to call a method that does not exist. The attempt was made from the following location:
io.github.resilience4j.circuitbreaker.configure.CircuitBreakerConfiguration.createCircuitBreakerRegistry(CircuitBreakerConfiguration.java:141)
The following method did not exist:
io.github.resilience4j.circuitbreaker.CircuitBreakerRegistry.of(Ljava/util/Map;Lio/github/resilience4j/core/registry/RegistryEventConsumer;Lio/vavr/collection/Map;)Lio/github/resilience4j/circuitbreaker/CircuitBreakerRegistry;

Hi @cgarman1
Are you using spring-cloud-dependencies as a dependecyManagement?
I got same issue with spring-cloud-dependencies because they use 1.1.0 version in spring-cloud-circuitbreaker-resilience4j. Following is my pom.xml.

  <dependencies>
    <dependency>
      <groupId>io.github.resilience4j</groupId>
      <artifactId>resilience4j-spring-boot2</artifactId>
      <version>1.3.0</version>
    </dependency>
  </dependencies>

  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>org.springframework.cloud</groupId>
        <artifactId>spring-cloud-dependencies</artifactId>
        <version>Hoxton.SR1</version>
        <type>pom</type>
        <scope>import</scope>
      </dependency>
    </dependencies>
  </dependencyManagement>

But currently we cannot exclude specific dependencies from dependencyManagement. Please see https://issues.apache.org/jira/browse/MNG-5600.
So I think we need to add our dependencies manually to override as you did.

[RobWin]

The issue is in the BulkheadAspect.

 if (CompletionStage.class.isAssignableFrom(returnType)) {
            // threadPoolBulkhead.executeSupplier throws a BulkheadFullException, if the Bulkhead is full.
            // The BulkheadFullException is not  handled correctly by the FallbackDecorator.
            return threadPoolBulkhead.executeSupplier(() -> {
                try {
                    return ((CompletionStage<?>) proceedingJoinPoint.proceed())
                        .toCompletableFuture().get();
                } catch (ExecutionException e) {
                    throw new CompletionException(e.getCause());
                } catch (Throwable e) {
                    throw new CompletionException(e);
                }
            });
        }

In the case your method returns a CompletableFuture or CompletionStage:
The CircuitBreaker Aspect returns a failed future which is completed exceptionally with a CallNotPermittedException when the CircuitBreaker is open. Retry and RateLimiter behave similar.

The ThreadPoolBulkhead is different, because it does not return a CompletionStage when a task could not be submitted, because the Bulkhead is full. The ThreadPoolBulkhead throws a BulkheadFullException instead. Analog to the ThreadPoolExecutor which throws the RejectedExecutionException.

threadPoolBulkhead.executeSupplier throws a BulkheadFullException, if the Bulkhead is full.
The BulkheadFullException is not handled correctly by the FallbackDecorator.

The BulkheadAspect can rethrow the BulkheadFullException or convert it to a failed future? What do you think a user expects? If no fallback method is configured, does a user expect a failed future or a RuntimeException when using the Bulkhead annotation?

I prefer to return a failed future.

[RobWin]

Problem is fixed in 1.4.0-SNAPSHOT.
But I will release it as 1.3.1 as soon as possible

[cgarman1]

Didn't notice this before but it looks like both 1.2.0 and 1.3.0 still use a couple of the 1.1.0 modules...which now produces a boot startup error with 1.3.0

An attempt was made to call a method that does not exist. The attempt was made from the following location:
io.github.resilience4j.circuitbreaker.configure.CircuitBreakerConfiguration.createCircuitBreakerRegistry(CircuitBreakerConfiguration.java:141)
The following method did not exist:
io.github.resilience4j.circuitbreaker.CircuitBreakerRegistry.of(Ljava/util/Map;Lio/github/resilience4j/core/registry/RegistryEventConsumer;Lio/vavr/collection/Map;)Lio/github/resilience4j/circuitbreaker/CircuitBreakerRegistry;

Hi @cgarman1
Are you using spring-cloud-dependencies as a dependecyManagement?
I got same issue with spring-cloud-dependencies because they use 1.1.0 version in spring-cloud-circuitbreaker-resilience4j. Following is my pom.xml.

  <dependencies>
    <dependency>
      <groupId>io.github.resilience4j</groupId>
      <artifactId>resilience4j-spring-boot2</artifactId>
      <version>1.3.0</version>
    </dependency>
  </dependencies>

  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>org.springframework.cloud</groupId>
        <artifactId>spring-cloud-dependencies</artifactId>
        <version>Hoxton.SR1</version>
        <type>pom</type>
        <scope>import</scope>
      </dependency>
    </dependencies>
  </dependencyManagement>

But currently we cannot exclude specific dependencies from dependencyManagement. Please see https://issues.apache.org/jira/browse/MNG-5600.
So I think we need to add our dependencies manually to override as you did.

Ah ok...yeah I missed that...yes that is exactly what I did/have

[cgarman1]

The issue is in the BulkheadAspect.

 if (CompletionStage.class.isAssignableFrom(returnType)) {
            // threadPoolBulkhead.executeSupplier throws a BulkheadFullException, if the Bulkhead is full.
            // The BulkheadFullException is not  handled correctly by the FallbackDecorator.
            return threadPoolBulkhead.executeSupplier(() -> {
                try {
                    return ((CompletionStage<?>) proceedingJoinPoint.proceed())
                        .toCompletableFuture().get();
                } catch (ExecutionException e) {
                    throw new CompletionException(e.getCause());
                } catch (Throwable e) {
                    throw new CompletionException(e);
                }
            });
        }

In the case your method returns a CompletableFuture or CompletionStage:
The CircuitBreaker Aspect returns a failed future which is completed exceptionally with a CallNotPermittedException when the CircuitBreaker is open. Retry and RateLimiter behave similar.

The ThreadPoolBulkhead is different, because it does not return a CompletionStage when a task could not be submitted, because the Bulkhead is full. The ThreadPoolBulkhead throws a BulkheadFullException instead. Analog to the ThreadPoolExecutor which throws the RejectedExecutionException.

threadPoolBulkhead.executeSupplier throws a BulkheadFullException, if the Bulkhead is full.
The BulkheadFullException is not handled correctly by the FallbackDecorator.

The BulkheadAspect can rethrow the BulkheadFullException or convert it to a failed future? What do you think a user expects? If no fallback method is configured, does a user expect a failed future or a RuntimeException when using the Bulkhead annotation?

I prefer to return a failed future.

So if no fallback is configured, to me the existing behavior is fine. However if I have a fallback method configured, I expect it to be called (instead of throwing BulkheadFullException) and let me decide what to do. Basically my use case if that I'm retrofitting an existing aggregation service and want to add this functionality without impacting multiple layers of the application with additional try/catch logic. I'm wanting to provide some process isolation via separate thread pool (via @bulkhead) and in the case that it is full, I want to be able to dummy out a response for my downstream service call and return what data I did get from the other service calls back to my client (that is what I am using the fallback method for...i'm creating my own future there). To me the annotation should not make any assumptions about what to return...it should just see that since there was a fallback method configured I will call that method and let the developer determine what gets returned from there.

[RobWin]

The existing Bulkhead Aspect behavior has been fixed to match the behavior of the CircuitBreaker, Retry and RateLimiter Aspects. That means if on don't have a fallback method the CompletableFuture which is returned will be completed exceptionally with a BulkheadFullException.
If you have a fallback method, it will be invoked correctly now.

[RobWin]

1.3.1 has been released.

[cgarman1]

Yes, that works as I expected. However one question...I assume that configuring fallback on @bulkhead that itself does not throw another error will cause @CIRCUITBREAKER to never get called, correct?

[RobWin]

Yes, the aspects decorate your method like an onion and have a preconfigured order. But can change the order application-wide.

Request -> Retry -> CircuitBreaker -> RateLimiter -> TimeLimiter -> Bulkhead -> method.

Exceptions propagate from right to left. Every decorator (proxy) has the possibility to recover from an exception (fallback). In your case you should add the fallback method to the CircuitBreaker. At least if you don't use Retry, but only CircuitBreaker, TimeLimiter and ThreadPoolBulkhead.

[cgarman1]

Yeah ok that's what I thought