Repository navigation
DISCUSSION: Should a breaker in half-open state care if call results received were initiated in half-open state? #217
Description
TL;DR: No (does not seem to add much value, for complexity)
Scenario
@Kharos rightly points out in #216, that a call placed through a circuit-breaker may return to a breaker in a different state from when the call commenced.
When the breaker is in a HalfOpen state, it is intended that only limited trial calls are placed, to determine the health of the underlying system. However, it is possible for a breaker in HalfOpen state to also receive return results from calls which started earlier when the breaker was still closed. If responding very slowly, these 'overhanging' calls could have remained in flight all through the time the breaker transitioned to Open, then to HalfOpen, and hit the breaker with their result when it is in the HalfOpen state.
How should the breaker handle a result received in the HalfOpen state that was not initiated during the HalfOpen state? Should it care?
Background: A success result received in HalfOpen state re-closes the breaker; a fail result received in HalfOpen state breaks it again.
Observation
The scenario would typically occur if the breakDuration configured on the breaker is less than the typical timeout of calls placed to the underlying system. For instance, calls time out at 30 seconds, but you configure only a 10-second break on the circuit-breaker. Setting a breakDuration as long as call timeout would typically clear in-flight calls.
Initial thoughts
Polly should 'keep things simple' and not care whether a success result received during HalfOpen state was initiated during HalfOpen state or not.
Would distinguishing the state under which the call was initiated add significant value? Hard to reason at this level, about undefined systems. To re-close the circuit only on a success result whose call was initiated during the HalfOpen state, risks re-closing the circuit 'later than necessary' if the underlying system has genuinely recovered. To re-close the circuit on any success result risks re-closing the circuit 'too early' (if it is still in intermittent health), but continuing circuit metrics can detect this and break the circuit again if necessary.
in the absence of more decisive arguments ... to begin to care about it makes for extra complexity, all of which has to be explained to users and/or understood by those later approaching the code -> keep things simple!
Current behaviour
Polly does not care whether a call result received during HalfOpen state was initiated during HalfOpen state.
Comparison
Hystrix CircuitBreaker takes no account: it does not track the circuit state under which calls were initiated; any success on a half-open breaker closes the circuit again. Documentation does not remark.
Views?
Open to views, as these are complex systems to reason about.
Activity
Kharos commentedon Jan 29, 2017
Works for my use case.
What I really need is a possibility to treat web requests that take longer than some amount X of time as a failure (as large numbers of these indicate that the server is not healthy). I am able to implement this using Polly as it is now and a 'soft' timeout in our async callback - the task returns after this timeout but the actual requests continues. The timeout value will be shorter than the 'circuit broken' time, so tracking of requests would add no benefit for me.
[Edit] That was probably hard to understand, so let me rephrase :-D. My 'soft' timeout will ensure that all async calls Polly will see are shorter than the circuit broken time, even if the 'hard' http timeout is longer.
reisenberger commentedon Jan 29, 2017
@Kharos Obviously
HttpClient(if using) has its own built-in soft timeout onSendAsync(), throws aTaskCanceledExceptionon timeout (perhaps not uniquely distinguishable between a cancellation and a timeout). Re:have you seen the Polly
TimeoutPolicy? ItsTimeoutStrategy.Optimisticis the same mechanismHttpClientuses underneath (timing-out cancellation token), but it throws a customTimeoutRejectedException, giving you something you can treat more clearly as a failure. Wrap aTimeoutPolicyinside theCircuitBreakereither manually or with the new PolicyWrap, have the circuit-breaker handle thoseTimeoutRejectedException, and you have precisely the mechanism to treat web requests exceeding duration as health indicators that can eg be configured to trigger a circuit-break. (TimeoutPolicywithTimeoutStrategy.Optimisticalso transmits its timing-out cancellation token into the governed call, so you achieve the same soft cancellation onHttpClient.)Kharos commentedon Feb 1, 2017
Looks good, using your forked branch my test with two parallel half-open requests now passes: One request is allowed through, the other throws, and the onBreak callback is only invoked once. I also wrote an additional test to check the behavior when the first half-open request takes a very long time, and after waiting for the break duration one (and only one) other request is let through ð.
Kharos commentedon Feb 1, 2017
About the timeouts, I am not sure that what you propose will cover my use case.
I am sending requests to a downstream server. 99% of all requests return within 4 seconds, but some requests can take significantly longer (10-20 seconds) because they access very expensive resources. So if a few requests take longer than 4 seconds I want to let them run to completion (and I have to set the HttpClient timeout pretty large to make this work), but if a significant number of requests takes longer than 4 seconds this is a sign that the downstream is unhealthy and I want the circuit to break.
My currrent solution is to use a Task<Task> in the polly circuit breaker: The wrapped inner task is the async HttpClient task, the outer task returns if either the inner task succeeds or if 4 seconds pass (using Task.WhenAny with the inner task and a delay task). I use Polly.HandleResult and return true to the circuit breaker if the inner task did not complete successfully (meaning it either failed or it is still running); after receiving the result from the circuit breaker I await the inner task to get the final result of the request. The benefit of this complicated implementation is that the circuit will break after 4 seconds when the downstream stops responding, even though the requests are all still in flight.
My code looks a bit complicated but it works. If there is a better was to do this using Polly's timeouts then please tell me :) but my understanding is that they will abort the inner task, which is not what I want.
[Edit] I realize that the proper solution would probably be two circuit breakers with different timeouts, but the URLs are very similar and I can't easily find out if a request is in the slow or in the fast category before I send the request...
reisenberger commentedon Feb 1, 2017
@Kharos ð re test confirmation. Those tests match the (unit and integration) tests I added to the Polly codebase too, so we have regression coverage.
@joelhulen and I will aim to push out new Nuget pkg in next few days. (May squeeze other outstanding PRs into it, to avoid frequent mini releases.)
reisenberger commentedon Feb 1, 2017
@Kharos With your nuanced configuration where you want to detect that tasks have exceeded a certain time limit but not abort them if they do, I agree that you need a custom implementation such as the one you describe, rather than the off-the-shelf
TimeoutPolicy.reisenberger commentedon Feb 2, 2017
Closing - implemented as recommendation which was set out, in #223