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

[API Proposal]: Expose Resilience HTTP Message Handler #4759

Closed
kmcclellan opened this issue Nov 27, 2023 · 4 comments · Fixed by #4858
Closed

[API Proposal]: Expose Resilience HTTP Message Handler #4759

kmcclellan opened this issue Nov 27, 2023 · 4 comments · Fixed by #4858
Assignees
Labels
api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-resilience enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@kmcclellan
Copy link

Background and motivation

I'm attempting to migrate from Microsoft.Extensions.Http.Polly to Microsoft.Extensions.Http.Resilience (see dotnet/docs#40841).

However, in my code base there are some scenarios where I want to bypass IHttpClientBuilder and instantiate an HTTP delegating message handler directly. The former library exposes a public PolicyHttpMessageHandler for this purpose. Can we consider exposing this library's handler?

To motivate, I'll illustrate two reasons I have found for bypassing the builder.

  1. To prevent duplicate handlers from being registered when AddResilienceHandler is called more than once.
  2. To configure multiple resilience handlers at once using the client name.

These can both be accomplished with a custom implementation of IConfigureNamedOptions<HttpClientFactoryOptions>:

class ConfigureHttpResilience : IConfigureNamedOptions<HttpClientFactoryOptions>
{
        public void Configure(string? name, HttpClientFactoryOptions options)
        {
            if (name != null)
            {
                // Create a custom resilience pipeline using the client name.
                var pipeline = GetPipeline(name);

                // Can't do this since ResilienceHandler is not public!
                options.HttpMessageHandlerBuilderActions.Add(
                    builder => builder.AdditionalHandlers.Add(new ResilienceHandler(x => pipeline)));
            }
        }
}
public static IServiceCollection AddHttpResilience(this IServiceCollection services)
{
    // "Try" prevents duplicates if this method is called more than once.
    services.TryAddEnumerable(
        ServiceDescriptor.Singleton<IConfigureOptions<HttpClientFactoryOptions>, ConfigureHttpResilience>());

    return services;
}

API Proposal

public class ResilienceHttpMessageHandler : DelegatingHandler
{
    public ResilienceHttpMessageHandler(ResiliencePipeline<HttpResponseMessage> pipeline)
    {
        throw new NotImplementedException();
    }

    public ResilienceHttpMessageHandler(Func<HttpRequestMessage, ResiliencePipeline<HttpResponseMessage>> pipelineProvider)
    {
        throw new NotImplementedException();
    }

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        throw new NotImplementedException();
    }
}

API Usage

options.HttpMessageHandlerBuilderActions.Add(x => x.AdditionalHandlers.Add(new ResilienceHttpMessageHandler(pipeline)));

Alternative Designs

No response

Risks

Future changes to the handler's public constructors are more likely to be breaking.

@joperezr
Copy link
Member

Thanks for logging the proposal @kmcclellan 😃

Tagging @martintmk and @geeknoid so that they can have a closer look at the proposal.

@joperezr joperezr added area-resilience api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed untriaged labels Nov 27, 2023
@geeknoid geeknoid added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Nov 27, 2023
@martintmk
Copy link
Contributor

@joperezr LGTM

I was even thinking about exposing it too, but decided until there is a real use-case for this.

kmcclellan added a commit to kmcclellan/dotnet-extensions that referenced this issue Nov 28, 2023
kmcclellan added a commit to kmcclellan/dotnet-extensions that referenced this issue Nov 28, 2023
@joperezr joperezr added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 28, 2023
@wjrogers
Copy link

wjrogers commented Dec 1, 2023

Please consider exposing ResilienceKeys.RequestMessage, too. If you don't want to make the key itself public, you could provide a GetRequestMessage(this ResilienceContext) extension method? Having access to the request in custom strategy logic is very useful.

@IGx89
Copy link

IGx89 commented Dec 6, 2023

Here's a use case I believe from the official docs: https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines#resilience-policies-with-static-clients. Currently attempting to follow that guidance using resilient pipelines but not having any luck so far.

geeknoid pushed a commit that referenced this issue Jan 6, 2024
@ghost ghost added the work in progress 🚧 label Jan 6, 2024
geeknoid pushed a commit that referenced this issue Jan 8, 2024
geeknoid added a commit that referenced this issue Jan 8, 2024
Fixes #4759

Co-authored-by: Martin Taillefer <[email protected]>
@ghost ghost removed the work in progress 🚧 label Jan 8, 2024
halter73 added a commit that referenced this issue Jan 9, 2024
Co-authored-by: Martin Taillefer <[email protected]>
Co-authored-by: martintmk <[email protected]>
Co-authored-by: Nikita Balabaev <[email protected]>
Co-authored-by: Martin Taillefer <[email protected]>
Fixes #4759
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-resilience enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants