[API Proposal]: Add IHttpClientBuilder to builder interfaces returned by AddStandardResilienceHandler(), AddStandardHedgingHandler()

[abatishchev]

Background and motivation

Currently the top-level API methods for Microsoft.Extensions.Http.Resilience are:

public static IHttpStandardResiliencePipelineBuilder AddStandardResilienceHandler(this IHttpClientBuilder builder);

public static IStandardHedgingHandlerBuilder AddStandardHedgingHandler(this IHttpClientBuilder builder);

Both return an inner builder:

public interface IHttpStandardResiliencePipelineBuilder
{
    string PipelineName { get; }
    IServiceCollection Services { get; }
}

public interface IStandardHedgingHandlerBuilder
{
    string Name { get; }
    IServiceCollection Services { get; }
    IRoutingStrategyBuilder RoutingStrategyBuilder { get; }
}

Each has a reference to IServiceCollection, but not to IHttpClientBuilder.

API Proposal

Update the builder interfaces with a reference to the original IHttpClientBuilder:

public interface IHttpStandardResiliencePipelineBuilder
{
  // ...
  IHttpClientBuilder HttpClientBuilder { get; }
}

public interface IStandardHedgingHandlerBuilder
{
  // ...
  IHttpClientBuilder HttpClientBuilder { get; }
}

API Usage

Currently, the user has to store a reference to IHttpClientBuilder manually:

var httpClientBuilder = services.AddHttpClient(...);
httpClientBuilder.AddStandardResilienceHandler(...);
httpClientBuilder.AddStandardHedgingHandler(...);

Instead, this would be possible:

services.AddHttpClient(...)
        .AddStandardResilienceHandler(...)
        .HttpClientBuilder // <-- the newly added property
        .AddStandardHedgingHandler(...);

Alternative Designs

No response

Risks

Minimal since both HttpStandardResiliencePipelineBuilder and StandardHedgingHandlerBuilder are currently private classes (records).

[joperezr]

cc @iliar-turdushev can you please take a look? Proposal seems straight forward, so if there aren't any concerns implementation-wise, we can mark it as ready-for-review.

My 2 cents here of course the ideal would have been for both of those AddStandard.. methods to return the httpClientBuilder, but they didn't and it would be a breaking change to change that. Given the state we are in, it isn't immediately obvious to me why calling a property is necessarily better than your workaround example where the caller just stores the http client builder.

[joperezr]

cc: @martintmk

[abatishchev]

@joperezr, @martintmk what should be the next step? If you agree, I'd like to give it a try and open a pull request with the change

[martintmk]

For the context, when we were initially designing the API, both IStandardHedgingHandlerBuilder and IStandardHedgingHandlerBuilder contained reference to IHttpClientBuilder.

In the API review, nesting builders was considered as anti-pattern and we removed the reference. I see the benefits though, so if everyone agrees we can introduce this change.

cc @geeknoid Your thoughts on this Proposal?

[abatishchev]

@geeknoid ping, please

[geeknoid]

Extending a public interface type would be a breaking change, right?

[abatishchev]

Technically, yes. But I think it's extremely unlikely anyone uses a non-default implementation of any builder interface. Such interfaces interfaces aren't a public contact but a standard way to implement the fluent/builder API.

[geeknoid]

@joperezr If you're OK with the potential breaking change, then I'm also OK with the change.

[joperezr]

I of course would rather not do a breaking change unless we absolutely have to, but as called out I also think this is very unlikely to break people. That said, I'm not really a SME on builder APIs, so I'll mark this as ready for review and check what experts like @halter73 think to make sure they agree this breaking change is okay.

[joperezr]

Actually, wait a sec, would this break if some code compiled against the older version of the interface (via an old package) but then ends up using a newer package with the new interface and doesn't re-compile?

(mainly asking as that would change my assessment of "unlikely to break people")

[RussKie]

Should we get advice from ARB folks?

[martintmk]

How about exposing HttpClientBuilder extension method instead? Usage would be very similar:

services.AddHttpClient(...)
        .AddStandardResilienceHandler(...)
        .HttpClientBuilder()
        .AddStandardHedgingHandler(...);

[abatishchev]

.HttpClientBuilder()

Wouldn't this be awkward and inconsistent comparing to the rest of the API? e.g. the property Services already exists and is widely used.

[halter73]

Technically, yes. But I think it's extremely unlikely anyone uses a non-default implementation of any builder interface. Such interfaces interfaces aren't a public contact but a standard way to implement the fluent/builder API.

This isn't something we've been super consistent with. MVC, Blazor and SignalR use interfaces for their builders. IApplicationBuilder, IEndpointRouteBuilder, IEndpointConventionBuilder and IHttpClientBuilder are all interfaces too, but AuthenticationBuilder, OptionsBuilder<TOption> don't implement any interfaces.

An upside of interfaces is that you can have a builder that implements multiple builders or otherwise augments the builder like WebApplication which implements IApplicationBuilder and IEndpointRouteBuilder, but it does make it harder to add anything to the builder particularly if you're targeting netstandard or net4xx where I don't think we can rely on C# 8 default interface methods (DIM). You could also argue the interfaces make it easier to write certain kinds of test cases, but I don't find that super compelling.

Actually, wait a sec, would this break if some code compiled against the older version of the interface (via an old package) but then ends up using a newer package with the new interface and doesn't re-compile?

(mainly asking as that would change my assessment of "unlikely to break people")

This would break implementers of the interface unless there is a DIM and the implementing type is loaded by a runtime that supports DIMs. It shouldn't break code simply calling interface methods unless those methods were removed. That said, I don't think it's worth accepting any level of breaking change just to save people from storing the IHttpClientBuilder in a variable.

[abatishchev]

It shouldn't break code simply calling interface methods unless those methods were removed

So this is not a breaking change, correct?

just to save people from storing the IHttpClientBuilder in a variable.

The existing API is less fluent, less convenient, more chatty. Hence the proposal.

[iliar-turdushev]

What if we add IHttpClientBuilder only on those .NET versions that support C# 8, i.e. support default interface property implementation:

public interface IHttpStandardResiliencePipelineBuilder
{
#if NET6_0_OR_GREATER
  IHttpClientBuilder HttpClientBuilder { get => null; }
#endif
}

Then HttpClientBuilder property will become available on .NET 6 and later. .NET Framework 4.6.2 will not support it though. Such a change will not break our customers.

[geeknoid]

@iliar-turdushev That sounds quite reasonable.

[RussKie]

What if we add IHttpClientBuilder only on those .NET versions that support C# 8, i.e. support default interface property implementation:

public interface IHttpStandardResiliencePipelineBuilder
{
#if NET6_0_OR_GREATER
  IHttpClientBuilder HttpClientBuilder { get => null; }
#endif
}

Then HttpClientBuilder property will become available on .NET 6 and later. .NET Framework 4.6.2 will not support it though. Such a change will not break our customers.

Mm.... This makes me concerned. Would this pass package validation?

[iliar-turdushev]

Mm.... This makes me concerned. Would this pass package validation?

@RussKie It seems that the validation passes. Please, take a look at the following PR #5215. All checks have passed. Does it mean that we are good? The HttpClientBuilder property I've added is Experimental though. It shouldn't affect the validation, right?

UPDATE Making the property stable breaks the build. After I removed the Experimental attribute the package validation started to fail, see #5220. We already have a package with the same issue. To fix it the warning is simply suppressed. Are we good to apply the same approach (suppress the warning) here?

[abatishchev]

@iliar-turdushev ping on this, please. who should we follow up with to respond to your latest update?

[joperezr]

Langversion is not tied to a TFM, but instead to the tooling which can vary. I'm not a fan of forking the API surface between TFMs to be honest unless we absolutely have to which doesn't seem to be the case here. Package Validation won't fail with the proposal because the two TFMs aren't compatible with each other (meaning you can't compile against net6.0 but then execute against net462)

[joperezr]

@abatishchev I really appreciate the suggestion here, and I also believe that if we were to redo this we would have probably have these properties to begin with, but given we have already shipped and the potential breaking changes called above, I'm leaning to following @halter73's advice here (who is the SME on aspnetcore APIs) and not take the breaking change given the workaround is really straight forward (just store the httpclientbuilder in a variable). @iliar-turdushev or @abatishchev is there any scenario where doing the workaround wouldn't be possible? Mainly asking as that could influence whether or not we think we can take this.

[iliar-turdushev]

Langversion is not tied to a TFM, but instead to the tooling which can vary.

@joperezr AFAIK default interface implementation (DIM) relies on the runtime to identify whether DIM is supported or not. It is not supported on .NET Framework, and the support starts with .NET Core 3. I cannot find this information in the official documentation though.

@halter73 Could you, please, take a look at the https://github.com/dotnet/extensions/issues/5165#issuecomment-2151814041? If we use DIM and add a new property only for .NET 6 and later, it wouldn't break any of our customers, right?

What do you think about the approach? Yes, we'll have different API's for .NET and .NET Framework, but isn't it reasonable to introduce more convenient API for modern platforms (.NET)? Or is there any reason not to do that?

Thanks.

[halter73]

The API suggested in https://github.com/dotnet/extensions/issues/5165#issuecomment-2151814041 is fine from a breaking change perspective. We have precedence for this with APIs like IMemoryCache.GetCurrentStatistics() and IAuthorizationPolicyProvider.AllowsCachingPolicies. We generally try to avoid having different APIs for different TFMs, but it wouldn't be the first time. I personally don't think this is worth it, but it's not a huge deal either way, and I can appreciate the convenience of not having to define a variable.

[iliar-turdushev]

@abatishchev We are considering adding the HttpClientBuilder property. Could you, please, look at the summary below and confirm it? I'm also wondering what's the real use-case for the HttpClientBuilder property. It's unlikely that somebody will register both the resilience and the hedging handlers in a single HttpClient. Thank you.

Summary of the discussion above

Currently, if you registers a resilience handler, then you cannot chain methods to add other resilience handlers. You have to store a reference to IHttpClientBuilder manually and use it in the following way:

var httpClientBuilder = services.AddHttpClient(...);
httpClientBuilder.AddStandardResilienceHandler(...);
httpClientBuilder.AddStandardHedgingHandler(...);

To fix this we can introduce IHttpClientBuilder HttpClientBuilder property to the interfaces IStandardHedgingHandlerBuilder, IHttpResiliencePipelineBuilder, and IHttpStandardResiliencePipelineBuilder, and use them for further method chaining:

services.AddHttpClient(...)
        .AddStandardResilienceHandler(...)
        .HttpClientBuilder // <-- the newly added property
        .AddStandardHedgingHandler(...);

[abatishchev]

hey @iliar-turdushev , I'm sorry for the delayed response, was on vacation past several days.

We are considering adding the HttpClientBuilder property

Awesome, thank you!!

Could you, please, look at the summary below and confirm it?

:shipit:

It's unlikely that somebody will register both the resilience and the hedging handlers in a single HttpClient.

To be honest, that was me :) I'm migrating from the previous generation of Polly but the docs are sparse, so I easily can imagine doing something wrong.

[iliar-turdushev]

Since it is unlikely that anybody will intentionally register both the resilience and the hedging handler in a single HttpClient, the only scenario that will benefit from the proposal is the following:

services
    .AddHttpClient(...)
    .AddStandardResilienceHandler(...) /* OR .AddStandardHedgingHandler(...) */
    .HttpClientBuilder // <-- the newly added property
    .AddResilienceHandler(/* configure, for example, chaos strategies */);

or vice versa:

services
    .AddHttpClient(...)
    .AddResilienceHandler(/* configure custom resilience strategies that, for example,
        add useful information to the ResilienceContext, as we do in the StandardHedgingHandler */)
    .HttpClientBuilder // <-- the newly added property
    .AddStandardResilienceHandler(...); /* OR .AddStandardHedgingHandler(...); */

To be able to achieve the scenarios above we'll need to introduce IHttpClientBuilder HttpClientBuilder property to the interfaces IStandardHedgingHandlerBuilder, IHttpResiliencePipelineBuilder, and IHttpStandardResiliencePipelineBuilder, as described here https://github.com/dotnet/extensions/issues/5165#issuecomment-2151814041.

@martintmk @geeknoid @RussKie @joperezr @abatishchev What do you think? Is it worth it? Thank you.

[RussKie]

Since it is unlikely that anybody will intentionally register both the resilience and the hedging handler in a single HttpClient...

What if someone does, what would it mean for the API? How would that use case pan out?

[RussKie]

Granted I don't have much experience with web API, but whilst I'm finding this snippet logical and easy to understand - "add HTTP client and get a builder, then configure the builder":

var httpClientBuilder = services.AddHttpClient(...);
httpClientBuilder.AddStandardResilienceHandler(...);
httpClientBuilder.AddStandardHedgingHandler(...);

...I can't say the same for this one - "add HTTP client and configure resilience handler from which you get HTTP client builder?":

services.AddHttpClient(...)
        .AddStandardResilienceHandler(...)
        .HttpClientBuilder // <-- the newly added property
        .AddStandardHedgingHandler(...);

@halter73 @amcasey @davidfowl is this idiomatic for the ASP.NET Core fluent API?

To me this sounds we're trying to save few keystrokes whilst trading the readability and clarity.

[iliar-turdushev]

What if someone does, what would it mean for the API? How would that use case pan out?

At the moment, we do nothing to prevent users from that, and if they do that they'll have an HttpClient with configured both the StandardResilienceHandler and the StandardHedgingHandler, which is not correct. It is possible to register them both but I cannot imagine a reasonable/valid use-case for that.