Polly V8: Public API Review (Finalization)

[martintmk]

Details on the issue fix or feature implementation

The follow-up of #1233 to finalize the API review. Contributes to #1301.

I transferred remaining unresolved comments here.

Confirm the following

• I started this PR by branching from the head of the default branch
• I have targeted the PR to merge into the default branch
• I have included unit tests for the issue/feature
• I have successfully run a local build

[martintmk]

Andrey:
I would expect Result getter to throw an exception if there was an exception.
similar to how Task.Result throws a captured exception.
but is this method even needed?

Martin:
I think we should avoid throwing exceptions from Outcome as it is low-level API used in high-perf scenarios and in delegates.

Juraj:
This is similar pattern that's used for HttpResponseMessage to check for successful status code. Also Cosmos DB SDK has similar implementation - you either work with DTOs directly (higher level) and get the exceptions, or you work with ResponseMessage (lower level) but need to check for the result explicitly using response.EnsureSuccessStatusCode().

[martincostello]

I prefer the HttpClient-style pattern too.

[andrey-noskov]

as discussed in a side-chat - we need to review all the public properties of this type and crystalize how customers are supposed to use it, e.g., the switch pattern and/or EnsureSuccess and/or some other approaches.
the HttpClient imo isn't a valid reference because the Client (a) always has a Result and (b) its EnsureSuccess method checks the result value, unlike this method.

[martintmk]

Little bit more context:

• This type is semantically/behaviorally almost identical to DelegateResult or PolicyResult from V7.
• The EnsureSuccess was added as a feature request by user (EnsureSuccess for PolicyResult #1238). We didn't want to add any new API to V7 so we included this method in V8.

[joelhulen]

I also prefer the EnsureSuccess approach. It doesn't matter whether you're expecting a result. You can also check EnsureSuccess to make sure that a void method completed without errors.

[martintmk]

What do you think about renaming EnsureSuccess to ThrowIfException to make the behavior more clear?

[andrey-noskov]

(as I suggested this change) I believe this change will make the contract cleaner: (1) the Throw part informs that the method can throw an exception and (2) the IfException explain the condition under which it can be thrown.

[codecov]

Codecov Report

Merging #1507 (41c62b9) into main (94db492) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1507   +/-   ##
=======================================
  Coverage   84.11%   84.11%           
=======================================
  Files         278      278           
  Lines        6586     6586           
  Branches     1026     1026           
=======================================
  Hits         5540     5540           
  Misses        837      837           
  Partials      209      209           

Flag	Coverage Δ
linux	84.11% <ø> (ø)
macos	84.11% <ø> (ø)
windows	84.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[andrey-noskov]

what is the purpose of this hint?

[martintmk]

This value is generated by the retry strategy and respects the the retry options and current attempt number.
If generator is not able to extract the delay from the outcome it can return this value as a next delay attempt.

[andrey-noskov]

I think it would better to either (1) change the contract to allow generator to return null as a signal that the built-in backoff strategy should be used or (2) expose the built-in backoff strategies to allow customers to use them in their generators

[andrey-noskov]

while I find this helper useful, I believe if we replace it with an extension for Outcome, it will enable more use-cases

var task = Outcome.FromResult("dummy").AsValueTask()

we should also be careful about the naming - ValueTask has a different contract to Task, and so not confuse anyone, I believe it would be better to use the "ValueTask" term as opposed to simple "Task"

[martintmk]

See #1328 where we discuss this API (and AsValueTask as well).

The following comment exaplains why we stuck with FromResultAsTask:

#1328 (comment)

[juraj-blazek]

Consider renaming to FromResultAsValueTask to make clear what it returns

[andrey-noskov]

from what I understand this is a factory like type which returns new instances on every call - because they are ValueTasks

Returning new instances from getter is considered bad practice.
I quickly googled to prove my point and found this gorgeous explanation https://stackoverflow.com/questions/2101646/is-object-creation-in-getters-bad-practice. see the accepted answer explaining the best practices compiled from a couple of well known champions

my proposal is to change the properties to methods, ie. PredicateResult.True()

[martintmk]

I am ok with this change, make it clear we are returning new instance.

@martincostello ?

[martincostello]

I don't use ValueTask often so maybe there's something I'm missing that means this won't work, but why don't we just return one we've already created?

public static class PredicateResult
{
    public static ValueTask<bool> True { get; } = new(true);
    public static ValueTask<bool> False { get; } = new(false);
}

[andrey-noskov]

ValueTasks can't be reused

[martincostello]

OK, then yes in that case let's change it to a method.

[andrey-noskov]

what is the role of this result? if a CB is open, there is no result, right?

[martintmk]

It returns result that opened the circuit. It's V7 API and we need to keep it.

[juraj-blazek]

Let's move this back to Polly and v8 will only throw BrokenCircuitException