Make ResilienceContextPool settable per pipeline

[cmeyertons]

Feature Request

Is your feature request related to a specific problem? Or an existing feature?

Specific problem -- in an Orleans grain, we should by default continue on the capturing context to avoid issues with grain losing required context to operate within the framework's custom task scheduler, etc.

This has been mentioned before #676 but received no traction, despite others mentioning Orleans specifically.

Describe your proposed or preferred solution

This repo did a great job defining a PolicyBase.DefaultContinueOnCapturedContext internal const. This issue should aim to publicize that variable as a static property.

^ the above refers to pre-v8 Polly. This issue will aim to address this in v8 using the ResiliencePipelines arch by making the ResiliencePipelineBuilder have a configurable ContextPool that supplies this behavior.

Describe any alternative options you've considered

Teams can create their own overloads to accomplish this behavior -- however, this is generally unsafe and requires creating & maintaining 20+ overloads. It also requires more contextual knowledge within the app to "know" to use the correct overload and leaves the door open to unsafe behavior by default (someone familiar w/ Polly will try to use the base Polly library)

Any additional info?

[martintmk]

I am always little hesitant exposing APIs that are static and can change the global behavior. This is that case. Have you considered the new v8 version?

Archiving this in v8 should be relatively straightforward. First define your custom pool that changes the default ContinueOnCapturedContextValue:

public class CustomResilienceContextPool : ResilienceContextPool
{
    public override ResilienceContext Get(ResilienceContextCreationArguments arguments)
    {
        if (arguments.ContinueOnCapturedContext is null)
        {
            arguments = new ResilienceContextCreationArguments(arguments.OperationKey, continueOnCapturedContext: true, arguments.CancellationToken);
        }

        return Shared.Get(arguments);
    }

    public override void Return(ResilienceContext context) => ResilienceContextPool.Shared.Return(context);
}

Now, assign this pool to ResiliencePipelineBuilder:

var builder = new ResiliencePipelineBuilder()
{
   ResilienceContextPool = new CustomResilienceContextPool();
}

var pipeline = builder.Build();

Now, whenever you execute any action using pipeline the new default will be respected. Please note that the ResilienceContextPool property is not yet exposed in the builder but it should be straightforward to introduce.

[martintmk]

cc @martincostello, do you see any other alternatives?

[martincostello]

Yeah, I would prefer we not add new behaviours to the previous API surface as it's basically "done" now and just for backwards compatibility, and instead look at ways to support this scenario in Polly.Core.

[cmeyertons]

Whoa, looks like I need to brush up on my Polly, I feel like a dinosaur using the static policy-based API (didn't know the DI-oriented resilience pipeline existed TBH, whoops)

With the enablement of DI in this pattern, this should be something I can wire up globally.

I'll give it a shot and post here when I'm done for posterity, appreciate the feedback!

[martincostello]

I feel like a dinosaur using the static policy-based API

Don't feel bad about it, we only released 8.0.0 2 weeks ago 😄

[cmeyertons]

@martincostello phew that does make me feel better 😄

@martintmk being able to configure the ResilienceContextPool for a given ResiliencePipelineBuilder would be great.

Without that functionality, I had to resort to some black magic reflection trickery to get it to work:

/// <summary>
/// This enforces continue on captured context to be true for all resilience strategies, which is vital for retaining Orleans grain runtime context
/// </summary>
private static ResiliencePipelineBuilder AddOrleansContext(this ResiliencePipelineBuilder builder)
{
    builder.AddStrategy(context => OrleansResilienceStrategy.Instance, new OrleansResilienceStrategyOptions());

    return builder;
}

private class OrleansResilienceStrategyOptions : ResilienceStrategyOptions
{ }

private class OrleansResilienceStrategy : ResilienceStrategy<object>
{
    private static readonly Action<ResilienceContext, bool> _continueOnCapturedContextSetter = typeof(ResilienceContext)
        .GetProperty(nameof(ResilienceContext.ContinueOnCapturedContext))
        !.GetSetMethod(true)
        !.CreateDelegate<Action<ResilienceContext, bool>>()
    ;

    private OrleansResilienceStrategy() { }

    public static OrleansResilienceStrategy Instance { get; } = new();

    protected override async ValueTask<Outcome<object>> ExecuteCore<TState>(Func<ResilienceContext, TState, ValueTask<Outcome<object>>> callback, ResilienceContext context, TState state)
    {
        _continueOnCapturedContextSetter(context, true); // always continue on captured context to not lose Orleans task scheduler in the pipeline

        return await callback(context, state);
    }
}

[cmeyertons]

@martincostello @martintmk PR up to make the pool settable on the builder, hope this helps

[martincostello]

Did #1693 completely resolve this issue (disregarding that we haven't released it yet), or is there more required?

[cmeyertons]

@martincostello yes, I believe this will completely resolve my issue given the test scenario I set up in the PR.

Please let me know when the next version is released and I will test against my code base. I will copy the working code into this issue for posterity.

[martincostello]

Thanks - in that case I'm going to close this issue.

If you would like to test the changes before the release, you can download NuGet packages from the artifacts on any of our builds in CI made since your change was merged.

[martincostello]

Polly 8.1.0 including this change is now available from NuGet.org - thanks again for your contribution!