[DRAFT] Nullability fixes for chaos outcome strategy

[peter-csala]

Pull Request

The issue or feature being addressed

Taking over the #1982 from @martintmk

Details on the issue fix or feature implementation

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.64%. Comparing base (010a8cf) to head (081b4b5).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2010   +/-   ##
=======================================
  Coverage   83.64%   83.64%           
=======================================
  Files         312      312           
  Lines        7105     7106    +1     
  Branches     1054     1053    -1     
=======================================
+ Hits         5943     5944    +1     
  Misses        789      789           
  Partials      373      373           

Flag	Coverage Δ
linux	83.64% <100.00%> (+<0.01%)	⬆️
macos	?
windows	83.64% <100.00%> (+<0.01%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[martincostello]

IMHO this isn't the sort of breaking change we should be "sneaking in" on people with a minor version

We made a mistake with the original API definition and fixing it should require a version 9.
I don't think it's worth creating a v9 solely to change this though.

If we still want to eventually fix it, then we should make the change if/when we have a legitimate need for a v9.

[peter-csala]

IMHO this isn't the sort of breaking change we should be "sneaking in" on people with a minor version

Let's see what will break.

Using OutcomeGenerator

OutcomeGenerator = new OutcomeGenerator<HttpResponseMessage>()
    .AddResult(() => new HttpResponseMessage(HttpStatusCode.InternalServerError))
    .AddException<HttpRequestException>()

No breaking change, the exact same code works in both cases.

Using async version

static ValueTask<HttpResponseMessage> GetAsync() => new ValueTask<HttpResponseMessage>(new HttpResponseMessage());

new ResiliencePipelineBuilder<HttpResponseMessage>()
    .AddChaosOutcome(new ChaosOutcomeStrategyOptions<HttpResponseMessage>
    {
        OutcomeGenerator = async static args => 
        { 
            return Outcome.FromResult(await GetAsync());
        } 
    });

No breaking change, the exact same code works in both cases.

Using sync version

var optionsOld = new ChaosOutcomeStrategyOptions<HttpStatusCode>
{
    OutcomeGenerator = _ => ValueTask.FromResult<Outcome<HttpStatusCode>?>(Outcome.FromResult(HttpStatusCode.OK))
};

var optionsNew = new ChaosOutcomeStrategyOptions<HttpStatusCode>
{
    OutcomeGenerator = _ => ValueTask.FromResult<Outcome<HttpStatusCode>>(Outcome.FromResult(HttpStatusCode.OK))
    //OutcomeGenerator = _ => Outcome.FromResultAsValueTask(HttpStatusCode.OK) //short form
};

That's the only place where the old code breaks with the new API.

[martincostello]

But haven't you changed a Nullable<T> to a T? The diff has lots of .Value usage that have been removed. That's a breaking change if so because the type has changed.

[peter-csala]

But haven't you changed a Nullable<T> to a T? The diff has lots of .Value usage that have been removed. That's a breaking change if so because the type has changed.

Yes, there is a lots of .Value removal. But they are on the Polly's implementation side, not on the Polly's usage side.
They are not affecting the consumer side of the API.

[martincostello]

Outcome<T> is a struct, so Outcome<T>? means Nullable<Outcome<T>> not "a nullable reference type of Outcome", so that's a breaking change if I'm interpreting the change to OutcomeGenerator in the public API correctly?

[peter-csala]

Outcome<T> is a struct, so Outcome<T>? means Nullable<Outcome<T>> not "a nullable reference type of Outcome", so that's a breaking change if I'm interpreting the change to OutcomeGenerator in the public API correctly?

When you directly use the Outcome in a synchronous fashion then yes that's a breaking change as I indicated in the Using sync version section.

This PR has changes to handle theOutcomeGenerator nicely. See the related implicit conversion operator.

[martincostello]

The operator would only fix in the case of source build though, right? If you update Polly in your own project and reference another library built with 8.3.0, then it won't work and you'd get a MissingMethodException/MissingMemberException.

The fact we need to change the public API files means we're changing/breaking the contract. I do not think we should do that.

[peter-csala]

The operator would only fix in the case of source build though, right? If you update Polly in your own project and reference another library built with 8.3.0, then it won't work and you'd get a MissingMethodException/MissingMemberException.

The fact we need to change the public API files means we're changing/breaking the contract. I do not think we should do that.

Shall I close this PR then or will you add a V9 label to it and let it diverge from the main?

[martincostello]

We can leave this open for discussion on exactly what we want to do about it, but my opinion is currently:

1. We could change the property to what it should have been originally, as it's not correct;
2. We should not make a breaking change to do so without bumping the major version;
3. This change is not valuable enough by itself for us to publish Polly 9.0.0.

Possible outcomes could be:

1. Accept we made a mistake and do nothing;
2. Intend to change it in the next major release, whenever that happens to be;
3. Create a Polly v9 that has no changes other than this breaking change.

My preference would be for 2, essentially, leave it as a TODO for an undetermined point in the future.

[martintmk]

I am gravitating towards being lenient on this single instance and do a breaking change by modifying the nullability annotations, while bumping minor version only.

My reasons:

• Runtime won't be affected. The library compiled with older version won't be affected, as nullability annotations don't have affect at runtime.
• The OutcomeGenerator usage won't be affected and I suspect that's how people set-up outcome strategy.
• Simpler usage. Changing nullability annotations simplifies the setup and allows use of Outcome utilities. While they library that used older version might not compile, the change will be trivial and will actually simplify the usage. We just need to document it.
• Returning null outcome was not possible, even though the annotations allowed it. No one can actually use this feature in current form.
• Chaos usage is not spread enough yet, better do it now as later it might hurt more.

Just my 2 cents and thoughts :)

[martincostello]

I won't be at a computer for a day or so to actually check to satisfy myself exactly which it is, but as I noted earlier as it's a struct not a class then isn't it actually changing being Nullable<T> or not rather than just an nullable annotation for a reference type? If that's the case, then it is a breaking change.

[martincostello]

Looking at the decompiled assemblies in ILSpy shows that this is indeed a binary-breaking change due to a change of type, not just a change to nullable annotations.

Viewing the code as C# 1.0 to make the semantics of the ? in this case obvious, you can see that in Polly.Core 8.3.1 the property is a ValueTask<Nullable<Outcome<TResult>>>:

Viewing the same from the package produced in this PR, it is now ValueTask<Outcome<TResult>>:

The key here is that because Outcome<TResult> is a struct, so ? means a different thing to if it were a reference type, and the former is breaking and the latter is not.

[peter-csala]

@martincostello Martin, I think it was never a question whether it is a breaking change or not. The question was more about whether we can squeeze this into the next release with minor version bump + proper documentation. Due to reasons that was previously listed @martintmk and myself are voting to treat this changeset as a bugfix to gain a more convenient API.

[martincostello]

The problem is it's binary breaking, and Martin's comments about the nullability not being visible at runtime don't apply because it's not just nullable annotations.

For me this isn't the right kind of change to make outside of a major version regardless of the benefits of doing so.

[martintmk]

Thanks Martin, it's indeed a binary breaking change to remove nullability from the struct.

Let's drop that change from the PR, we can still keep the others.

[peter-csala]

Thanks Martin, it's indeed a binary breaking change to remove nullability from the struct.

Let's drop that change from the PR, we can still keep the others.

Well the only change which would remain is this (as far as I can see):

internal Func<ResilienceContext, Outcome<TResult>> CreateGenerator()
{
      if (_factories.Count == 0)
      {
            // return _ => null; // old
            throw new InvalidOperationException("No outcome generators have been added.");

[martintmk]

Thanks Martin, it's indeed a binary breaking change to remove nullability from the struct.
Let's drop that change from the PR, we can still keep the others.

Well the only change which would remain is this (as far as I can see):

internal Func<ResilienceContext, Outcome<TResult>> CreateGenerator()
{
      if (_factories.Count == 0)
      {
            // return _ => null; // old
            throw new InvalidOperationException("No outcome generators have been added.");

The nullability contract allows returning null, so this change might not be necessary. However, using OutcomeGenerator and not having any outcomes seems wrong, so I would vote for this change.

What we need to fix is this piece of code:

Polly/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs

Line 28 in 197cc16

var args = new OnOutcomeInjectedArguments<T>(context, outcome.Value);

Returning null outcome results in exception as due to outcome.Value.

[peter-csala]

@martintmk Since you are back I'm closing this PR in favor of your.