Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature request]: Resilience Execution #2494

Open
caigen opened this issue Feb 14, 2025 · 5 comments
Open

[Feature request]: Resilience Execution #2494

caigen opened this issue Feb 14, 2025 · 5 comments

Comments

@caigen
Copy link
Contributor

caigen commented Feb 14, 2025

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

Polly already supports 2 main core concepts:

  • Resilience Policy
  • Resilience Pipeline

What I want to is:

I used it somewhere but wanted to contribute it back to Polly as I think it should be general and after it is added in polly, user can create resilience business logic easily & quickly. It can also be used for non-http related resilience.

Sample usage:

        // 1. Config Hedging & Timeout
        ResiliencePipelineBuilder<HttpResponseMessage> builder =
            new ResiliencePipelineBuilder<HttpResponseMessage>();
        builder = builder.AddHedging(
            new Polly.Hedging.HedgingStrategyOptions<HttpResponseMessage>
            {
                Delay = TimeSpan.FromSeconds(2),
                MaxHedgedAttempts = 1,
                ShouldHandle = async args =>
                {
                    return await Task.FromResult(args.Outcome.Result?.IsSuccessStatusCode ?? true);
                },
            });
        builder.AddTimeout(TimeSpan.FromSeconds(25));

        // 2. Build pipeline
        ResiliencePipeline<HttpResponseMessage> pipeline = builder.Build();

        // 3. Config ResilienceInvoker with multiple HttpClients
        FakeHttpClient fakeHttpClientA = new FakeHttpClient("A", 20);
        FakeHttpClient fakeHttpClientB = new FakeHttpClient("B", 5);

        // 4. runtime
        using HttpRequestMessage httpRequestMessage = new HttpRequestMessage();
        CancellationToken cancellationToken = new CancellationToken(false);

        // Invokers per request or we may have thread safety issue
        ResilienceInvoker<HttpResponseMessage, HttpRequestMessage> resilienceInvoker = new ResilienceInvoker<HttpResponseMessage, HttpRequestMessage>(
            pipeline,

            // Create one composite invoker internally
            new List<IInvoker<HttpResponseMessage, HttpRequestMessage>>
            {
                // Add multiple HttpClient here
                fakeHttpClientA,
                fakeHttpClientB
            });
        HttpResponseMessage httpResponseMessage = await resilienceInvoker.ExecuteAsync(httpRequestMessage, cancellationToken);
        Assert.Equal(HttpStatusCode.OK, httpResponseMessage.StatusCode);
        Assert.True(fakeHttpClientA.CalledNumber == 1 || fakeHttpClientB.CalledNumber == 1);

Describe the solution you'd like

/// <summary>
/// IInvoker interface: work together with Polly ResiliencePipeline as callback
/// See: https://www.pollydocs.org/pipelines/index.html .
/// </summary>
/// <typeparam name="TResult">The type of the result.</typeparam>
/// <typeparam name="TInput">The type of the input.</typeparam>
public interface IInvoker
    <TResult, TInput>
{
    /// <summary>
    /// Execute the request.
    /// </summary>
    /// <param name="request">The request.</param>
    /// <param name="cancellationToken">The cancellationToken.</param>
    /// <returns>The result.</returns>
    Task<TResult> ExecuteAsync(TInput request, CancellationToken cancellationToken);
}
classDiagram
    IInvoker <|-- CompositeInvoker
    IInvoker <|-- ResilienceInvoker
    class IInvoker{
      -//Execution abstraction
      -ExecuteAsync()
    }
    class CompositeInvoker{
      -IList _invokers
      +ExecuteAsync()
    }
    class ResilienceInvoker{
      +ResiliencePipeline _resiliencePipeline
      +CompositeInvoker _compositeInvoker
      +ExecuteAsync()
    }

Image

Additional context

IInvoker could be public.
CompositeInvoker could reuse current Composite infra in Polly Core.

No response

@martincostello
Copy link
Member

Thanks for opening this issue.

At first glance I'm not sure this is something we'd want.

Usage in the manner described is creating more overhead through the addition objects and need to enumerate the invocations, and Polly v8 was designed to be low allocation, so this would go counter to that.

Also the interface only allows for invocations with one specific signature, and doesn't have the flexibility of the additional overloads the abstract strategies have to accept a context and /or state as well as a CancellationToken.

Can you provide some further details about the motivating factors on why this is something you think would be a good addition to the core library? It just looks like syntactic sugar specific to how you happen to use Polly in your own application.

@caigen
Copy link
Contributor Author

caigen commented Feb 14, 2025

Thanks for reviewing this.

  1. For current ResiliencePipeline, I found it is a little bit complicated when I tried to combine State with Callback together. I want something trivial or near one line.

  2. In my case, it is one high availability usage case: e.g. when 1st service/solution has some issue, fall back to 2nd service/solution. The service could be http service or not. But most are http cases.

  3. yes, it is sugar also not. I want this sugar is stable and could make my code simpler and easily to maintain. And if is OO design, we can easily extend it also.
    It may be something nice to have. But if we have many users who need to write this similar thing again and again, we can provide one stable version for users to select to use it.
    E.g. Company A may need to write it. Company B may need to write it again. Team A and Team B in Company C may need to write it again.

  4. The interface or specific signature is just for enhancement not replacing current flexibility.

Not sure if this interface design is right approach, but the key motivation & goal from me is to Combine Execution & ResiliencePipeline & ResiliencePolicy easily.

@caigen
Copy link
Contributor Author

caigen commented Feb 14, 2025

Sharing some background about why I name it as IInvoker and provide ExecuteAsync for reviewing:

  • It provides one more generic abstraction like System.Net.Http.HttpMessageInvoker, not just for SendAsync but also for similar operations as ResiliencePipeline is generic enough for other operations also.

  • At same time, there is no IHttpMessageInvoker from .net currently for extension.

@martincostello
Copy link
Member

I'm afraid I'm still not convinced of the need to have this baked into the core library.

Maybe a more fully-fleshed out example (by which I mean something we could actually run) of this being used by real-world code would help with the sales pitch 😄.

@caigen
Copy link
Contributor Author

caigen commented Feb 17, 2025

Yes, let me make one draft PR on samples first to demo. Then we can review to see if it is something we should provide in lib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants