CircuitBreaker's Open state should return a faulted Task instead of throwing

[ismaelhamed]

Version Information
Akka 1.4.21

Describe the bug
CircuitBreaker's Open and HalfOpen states should return a faulted Task with the OpenCircuitException exception instead of throwing.

To Reproduce

circuitBreaker.WithCircuitBreaker(dangerousAsyncOperation)
    .PipeTo(Self, failure: exception => new Success.Failure(exception));

Expected behavior
Be able to handle the OpenCircuitException with just the PipeTo, without needing to add another ContinueWith to the mix.

Actual behavior
OpenCircuitException cannot be handled in the failure callback, and the actor restarts due to the unhadled exception.

Proposed fix
Return a faulted Task:

public override Task Invoke(Func<Task> body) => Task.FromException(
    new OpenCircuitException(_breaker.LastCaughtException, RemainingDuration()));

...instead of throwing

public override Task Invoke(Func<Task> body) =>
    throw new OpenCircuitException(_breaker.LastCaughtException, RemainingDuration());

[Aaronontheweb]

Agree - do we need to break the existing API in order to do this?

[ismaelhamed]

Not the API, but if someone is wrapping the calls to the CB in a try... catch (for whatever reason), that won't work anymore. I couldn't find anything like that in our code base except for this piece in the SnapshotStore:

try
{
    ReceivePluginInternal(message);
    _breaker.WithCircuitBreaker(() => DeleteAsync(saveSnapshotFailure.Metadata));
}
finally
{
    senderPersistentActor.Tell(message);
}

But I don't think the intent was to shield against any exception throwing in the CB but in the call to ReceivePluginInternal. In fact, here is the same code in scala, and AFAIK you cannot catch exceptions wrapped in a Future with a try... catch either.

[ismaelhamed]

@Aaronontheweb any thoughts?

[Aaronontheweb]

Looks good to me - let's go for it.

[Aaronontheweb]

Sorry for the delay on getting back to you on this @ismaelhamed - this change looks safe.