search

spring-cloud / spring-cloud-circuitbreaker

Spring Cloud Circuit Breaker API and Implementations
Apache License 2.0
329 stars 110 forks source link

Feign exception handling #97

Closed eduardo-bueno-ifood closed 3 years ago

eduardo-bueno-ifood commented 3 years ago

When using Feign, it is common to handle exceptions based on the response HTTP status code, for instance. Up to 2.0.0, if you didn't implement a fallback for your calls, the FeignException would be wrapped as the cause of a NoFallbackAvailableException. It seems that with 2.0.1 the cause of the NoFallbackAvailableExceptions is now a java.util.concurrent.ExecutionException for some reason, with the FeignException being the cause of that. It is becoming really difficult to check for HTTP status errors and the real cause of an Exception, not to mention that this change broke every code that handles exceptions based on the status code.

ryanjbaxter commented 3 years ago

Can you provide a complete, minimal, verifiable sample that reproduces the problem? It should be available as a GitHub (or similar) project or attached to this issue as a zip file.

eduardo-bueno-ifood commented 3 years ago

sample.zip Hopefully is simple enough

ryanjbaxter commented 3 years ago

@eduardo-bueno-ifood if you set spring.cloud.circuitbreaker.bulkhead.resilience4j.enabled=false that should return the old behavior

The bulkhead module is being brought in via another dependency so it is using Resilience4J bulkhead hence the different exception handling. The PR above excludes the dependency.

ryanjbaxter commented 3 years ago

I also thought about making a change in OpenFeign here https://github.com/spring-cloud/spring-cloud-openfeign/blob/master/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerInvocationHandler.java#L91

So if we don't have a fallback we provide our own fallback exception containing the status code, but it kind of seems redundant since the status code is there, it just depends on how the exception gets wrapped.

eduardo-bueno-ifood commented 3 years ago

Yes I have noticed Bulkhead was being configured by default, it seemed odd. Good catch but the problem still stands though if I want to use Bulkhead in the first place :stuck_out_tongue:

provide our own fallback exception containing the status code

How would you tackle this case where Bulkhead is also enabled?

Sorry I don't have any other idea off the top of my head.

ryanjbaxter commented 3 years ago

From my testing whether you include the bulkhead module or not now the wrapped exception is the same. New snapshot builds should be available in a few minutes, can you give it a try and let me know if you see something different.

eduardo-bueno-ifood commented 3 years ago

Just tested, looks good now. Thank you.