CircuitBreaker should not throw the original exception when the circuit is open

[tom59]

Feature description

The default behaviour for the micronaut CircuitBreaker is to throw the original exception when the circuit is open. https://github.com/micronaut-projects/micronaut-core/blob/v3.4.2/runtime/src/main/java/io/micronaut/retry/intercept/CircuitBreakerRetry.java#L96 This is very confusing and doesn't really make much sense in a production application. Why isn't CircuitOpenException thrown for all cases?

For example, if I have a client which is configured to retry once:

@Client
public interface SummaryClient {
    @Get("/summary")
    @CircuitBreaker(delay = "15s", attempts = "1", reset = "300s")
    protected abstract Single<Payload> getSummary()
}

I would expect the following in the logs:

io.micronaut.http.client.exceptions.ReadTimeoutException // First Attempt. Outbound timed out
io.micronaut.http.client.exceptions.ReadTimeoutException //Second Attempt. Outbound timed out
io.micronaut.retry.exception.CircuitOpenException // Circuit breaker is now open. No outbound calls were made
io.micronaut.retry.exception.CircuitOpenException // Circuit breaker is now open. No outbound calls were made

While we are getting this:

io.micronaut.http.client.exceptions.ReadTimeoutException // First Attempt. Outbound timed out
io.micronaut.http.client.exceptions.ReadTimeoutException //Second Attempt. Outbound timed out
io.micronaut.http.client.exceptions.ReadTimeoutException // Circuit breaker is now open. No outbound calls were made
io.micronaut.http.client.exceptions.ReadTimeoutException // Circuit breaker is now open. No outbound calls were made

I have a workaround to create a Fallback class for the client but that does seem to be really a hack.

@Fallback
public class FallbackSummaryClient extends SummaryClient {
    @Override
    protected Single<Summary> getSummary() {
        return Single.error(new CircuitOpenException("Circuit breaker is open."));
    }
}

[jameskleeh]

Can you describe why it doesn't make sense? What problems is it causing? How would a CircuitOpenException help you?

[graemerocher]

It is also unclear to me why this is a problem. Seems to be working exactly as designed

[tom59]

It is working as designed in the code, that's why I put it as an enhancement. The current circuit-breaker API is "lying" on why it fails. It will throw TimeoutException or HttpClientException when there is not outbound calls made. That is very confusing. When you see 100 TimeoutException being thrown, you are meant to believe there is something really wrong with the outbound which has timed out 100 times, but here it is likely to be that there were 2 timeouts and then the circuit breaker was just open for the 98 other calls. It would be better to be explicit on why it fails so that it is easier to get down to the problem (eg figuring out if it is a downstream issue or a circuit breaker configuration issue).

[graemerocher]

So this seems like an enhancement request whereby you want to specify to the circuit breaker which exceptions should be excluded from being rethrown.

[tom59]

yes that would work

[canastillo]

Hi! I'm looking for projects to make my first java contributions. I'd like to tackle this one if no one is working on it

[yawkat]

contributions welcome. nobody is working on this atm

[canastillo]

After setting up the environment and learning about how the code works, I realized I might have misunderstood the problem and the solution. I just want to confirm, while the Circuit breaker is open and this causes an exception, it's enough to only have one exception message (the io.micronaut.retry.exception.CircuitOpenException message), until the circuit is closed again and other type of exceptions may occur. Taking this into account, the suggestion is to add a way to tell the CircuitBreaker which exceptions should be excluded after they appeared once.

So, the new behaviour would be something like:

io.micronaut.http.client.exceptions.ReadTimeoutException // First Attempt. Outbound timed out
io.micronaut.http.client.exceptions.ReadTimeoutException //Second Attempt. Outbound timed out
io.micronaut.retry.exception.CircuitOpenException // Circuit breaker is now open. No outbound calls were made
// Circuit breaker is still open. No outbound calls were made and there is no exception message
// Circuit breaker is now closed
io.micronaut.http.client.exceptions.ReadTimeoutException // Third Attempt. Outbound timed out

Is this correct?

[dstepanov]

Yes, we should probably keep the previous behavior and enable a new one by introducing a new parameter on @CircuitBreaker

[canastillo]

I think I could use a little help. I added a new element excludeRethrown to the CircuitBreaker annotation, but I haven't found where to place the logic. I thought of adding a condition right here in the CircuitBreakerRetry, but I don't fully understand yet how to access the excludeRethrown array from here (note line 92):

I also thought of doing something similar to how excludes and includes logic is implemented, but I'm not sure that I have found where that code is.

[dstepanov]

@canastillo Look for the place where CircuitBreakerRetry is created, you can see how the annotation values are extracted.

[aprietop]

If I understand the request correctly, throwing the original exception when the circuit breaker is opened could be misleading because that exception is not occurring at that time, it is just the last exception captured before opening the circuit.

Having that in mind, I have created https://github.com/micronaut-projects/micronaut-core/pull/8222. The original exception is now always wrapped within the CircuitOpenException when the circuit is opened. It should provide the following flow:

io.micronaut.http.client.exceptions.ReadTimeoutException // First attempt. Outbound timed out
io.micronaut.http.client.exceptions.ReadTimeoutException //Second attempt. Outbound timed out
io.micronaut.retry.exception.CircuitOpenException 
... caused by io.micronaut.http.client.exceptions.ReadTimeoutException// Third attempt the circuit breaker is now open. No outbound calls made
io.micronaut.retry.exception.CircuitOpenException
... caused by io.micronaut.http.client.exceptions.ReadTimeoutException// Fourth attempt the circuit breaker is  still open. No outbound calls made
...
// Circuit breaker is now closed
io.micronaut.http.client.exceptions.ReadTimeoutException // Fifth attempt. Outbound timed out

Let me know if that suffices. @graemerocher @tom59

[graemerocher]

This is a breaking change and needs to be configurable

[aprietop]

@graemerocher I'm adding a new parameter to @CircuitBreaker as suggested by @dstepanov I've updated the PR https://github.com/micronaut-projects/micronaut-core/pull/8222