Polly: AdvancedCircuitBreaker allows additional Excute() in HalfOpen state when waiting for first Excute() to finish

[vgouw]

When AdvancedCircuitBreaker changes from Open state to HalfOpen state it will allow additional Excute() while waiting for the first Execute() to finish.

In case the Excute operation takes a long time to generate a (TimeOut) Exception this will allow many calls to pass through while waiting for the (TimeOut) Exception.

I would like to be able to set the number allowed Execute in HalfOpen state .

[Kharos]

I just hit the same problem. Our use case is using a circuit breaker in front of outgoing requests to a downstream server to prevent:

1. flooding of the downstream server with requests when it is unhealthy and returns HTTP 500
2. high resource consumption when downstream server cannot be reached (all requests will time out)

This behavior is pretty bad in case 2 as it may take a moment until the half-open requests will time out. In effect we will lose circuit breaker protection for at least several seconds after each [break duration] time.

I understand that the issue is not trivial, e.g. if we only allow a single half-open request and that request hangs it may keep the circuit open, so there needs to be some mechanism to e.g. allow a half-open request every [break duration] time even if the previous one has not yet returned. But this issue needs to be solved for async polly circuit breakers making sense with web requests.

[Edit] Additional edge case bug: The onBreak() callback is potentially called multiple times in that scenario, once for each failed hald-open request.
[Edit 2] Reading the code are a lot of other edge cases if requests are started when the policy is closed, the policy breaks, goes back to half-open, and only then the requests return a result. At the moment the circuit breaker would think that these requests were half-open testing requests. Not sure if that is an important case though...

[reisenberger]

@vgouw @Kharos We need to get this issue squashed. I'm just back from three days travel, and now looking at this.

[Kharos]

Great @reisenberger :+1:. Please tell me if I can support you - reading through the Polly codebase things looked a bit complex because the logic is distributed over multiple classes and no tracking of action seems to happen (i.e. there seems to be no way to know on a finished async call if that call was started closed or half-open), so I did not feel comfortable enough to propose a fix. But I would be happy to for example test your changes against my codebase.

[reisenberger]

No prob @Kharos , TDD-driven proof-of-concept already done yesterday, soon something pushable for review / comment / field testing :+1:.

( @Kharos @vgouw Have reviewed the various edge case qs / suggestions around transition out of HalfOpen state. Will respond and let's discuss further after engineering and validating a simple-as-possible (but not simpler) implementation! )

[reisenberger]

@vgouw @Kharos Thank you for highlighting this issue.

Bug fix(es), unit tests, and full integration tests (deterministically controlling parallel concurrent requests to simulate the scenarios) all now pushed to https://github.com/reisenberger/Polly/tree/b505pre-BreakerFixes . Specs aim to have simulated the scenarios thoroughly, but review and field report from your environment _always_ welcome.

Feel free to review/test from https://github.com/reisenberger/Polly/tree/b505pre-BreakerFixes ; will push this also as a v5.0.5-pre nuget package soon, may work another minor circuit-breaker change #207 into same.

@Kharos Thanks for this insightful angle:

if we only allow a single half-open request and that request hangs it may keep the circuit open, so
there needs to be some mechanism to e.g. allow a half-open request every [break duration] time

:+1: good solution, have adopted this. This is also how Hystrix approaches the problem.

@Kharos Re:

Additional edge case bug: The onBreak() callback is potentially called multiple times
in that scenario, once for each failed hald-open request.

Locks around the transition from HalfOpen to Open again, and that locked section _always_ transitioning the circuit out of HalfOpen before releasing the lock, mean that transitions from HalfOpen to Open couldn't be causing this (duplicate invocations of onBreak()). However, a thorough code review followed by integration tests suggested then proved that long-running requests, started when a circuit was closed, but hitting it with further failures later when it had already opened, could cause duplicate invocations of onBreak(). This issue also squashed, with accompanying integration tests. Thanks @Kharos for the commentary about long-running request edge-cases, which pointed to this.

[Kharos]

Awesome, you guys are fast :+1:!
I will be unable to test in the next two days as I am on a conference, but I will make sure to take a close look Wednesday morning European time.