tmenier / Flurl

Fluent URL builder and testable HTTP client for .NET
https://flurl.dev
MIT License
4.21k stars 388 forks source link

Named Clients + Configuration Overhaul #770

Closed tmenier closed 10 months ago

tmenier commented 1 year ago

[This should be the last major enhancement for 4.0. 🎉 Feedback is strongly encouraged. Description may go though a few iterations, so anyone interested in these changes is also encouraged to subscribe to this issue via the link to the right.]

Motivation

Flurl has long supported a "clientless" usage pattern, where developers can literally type a URL string and simply "call" it, without ever worrying about creating or managing client instances. To be clear, Flurl will continue to support this model forever, but as the .NET world (especially ASP.NET Core) has shifted more and more a DI-centric paradigms, the "explicit" client pattern has gained more traction, and there have been many requests to make Flurl more DI-friendly and generally work a little more like .NET's IHttpClientFactory.

Meet the New Interfaces

IFlurClientCache

A container for named IFlurlClients. Recommended DI usage is to bind as a singleton to FlurlClientCache (the default implementation) and inject into service classes, where it can be used in much the same way as IHttpClientFactory's named clients pattern.

Methods:

IFlurlClientBuilder

A builder for fluently configuring IFlurlClients.

Methods:

Settings and Headers properties are also defined (the default implementation applies them directly to the client being built), and all related fluent extension methods available on IFlurlClient (WithSettings, WithHeaders, etc.) are also available on the builder.

An Overhauled FlurlHttp

While most responsibilities for "global" configuration and caching are moving to IFlurlClientCache, which can be more easily managed by a DI container, the clientless model still needs to reference some implicit global/static context in order to function. This is now FlurlHttp's sole responsibility.

Methods:

Properties:

Breaking Changes to 3.x

[Concept-focused and not necessarily comprehensive. A more detailed list will be provided in the release notes.]

tmenier commented 1 year ago

UPDATE: the changes described here were released in prerelease 5 but are superceded by those in the next comment, to be released in pre6.

769 exposed a problem with removing factories: SocketsHttpHandler allows you to do more than the standard HttpClientHandler, but isn't supported on all platforms supported by Flurl, and I'm proposing taking away the ability to swap it in. As much as I like to avoid platform-sniffing, I think it makes sense here.

If your project targets .NET 6 or higher, you'll get a SocketsHttpHandler and this method signature:

IFlurlClientBuilder.ConfigureInnerHandler(Action<SocketsHttpHandler> configure)

For all other target platforms, you'll get an HttpClientHandler and this method signature:

IFlurlClientBuilder.ConfigureInnerHandler(Action<HttpMessageHandler> configure)

I want the above method to be as easy and safe to use as possible, which is why I opted against some generic form of ConfigureInnerHandler or one that requires casting. I'm open to feedback on that design decision.

tmenier commented 12 months ago

Per the previous comment, #769 was addressed in prerelease 5 via platform-sniffing and defaulting to SocketsHttpHandler on supported platforms. But that solution exposed #772, and has a potentially bigger problem: some platforms (most notably browsers) will "pass" the .NET 6+ check but would throw runtime errors when attempting to use SocketsHttpHandler. Under the hood, HttpClientHandler defers work to SocketsHttpHandler on supported platforms anyway, so it seems like the best/safest default in all cases. The only case I can think of where you'd want to switch is if you need to configure properties on SocketsHttpHandler that aren't available on HttpClientHandler.

So there will be a prerelease 6 with the following updates:

The original description of this issue has been updated to reflect the most recent changes.

tmenier commented 11 months ago

Update: IFlurlClientCache now implements ISettingsContainer and IHeadersContainer, hence has Settings and Headers properties and all the extension methods that come with them, rather than just a WithSettings method. Description updated.

tmenier commented 11 months ago

Update: IFlurlClientCache.ConfigureAll is being renamed to WithDefaults. I've struggled with naming this one (and am open to other suggestions before the final release), but ConfigureAll sounds like it would apply to clients already in the cache, which it does not. The intent is to call it up front to set up defaults that can be overridden; I think calling it later would be a bad practice.

ngoquoctoandev commented 11 months ago

The era of .NET 8 has come, and there are not many systems/software using the old version of .NET anymore. I think SocketsHttpHandler should still be added as an additional option for those using the latest version of .NET

tmenier commented 11 months ago

@ngoquoctoandev Agreed, that's why UseSocketsHttpHandler has been added. Trying to use it implicitly via platform-sniffing led to #772, which is why I made it opt-in. And what a lot of people don't realize is HttpClientHandler (the default) defers all work to SocketsHttpHandler on supported platforms anyway. I didn't need to re-invent that wheel (figuring out when to use it) because MS already took care of it:

https://github.com/dotnet/runtime/blob/764d3e0cfab629bb6e594f3399c9ba7362a621c3/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs#L13-L25

satishviswanathan commented 10 months ago

@tmenier - I'm using Flurl library in more than 30+ microservices and the way I had implemented was having a thin rest service which abstracts some cross cutting concerns for all of them. Within this rest service I have been using the IFlurlClientFactory to either get a new instance or a cached instance of HttpClient for the given url.

OldCode

// RestService is registered in DI as singleton.
public class RestService
{

   public GetApiResponse(string baseUrl, IEnumerable<string> urlSegments)
  {
      //resolved from DI
     var httpClient =  _flurlClientFactory.CreateHttpClient().Get(serviceOptions.Url);
     var response =  await httpClient .SendAsync(...);
    return response;
  }
}

As per the new implementation I understand the recommended approach is to use the IFlurlClient from the DI container. At this stage I'm not in a position to make this change on all of them since they are all in production now and it would require lot of effort to make this change. I was trying to see a way to upgrade the library without the recommended DI method using the FlurlHttp implementation.

My plan was to use the FlurlClientBuilder within my rest service as follows.

New Code

public class RestService
{

   public GetApiResponse(string baseUrl, IEnumerable<string> urlSegments)
  {
     var cli =  FlurlHttp.ConfigureClientForUrl("https://api.com/some/path")
    .WithSettings(s => s.Timeout = TimeSpan.FromSeconds(123)).Build();
     var response = await cli.SendAsync(...);
    return response;
  }
}

In the new implementation I'm using FlurlHttp to get a client for a given url. However when I call the FlurlHttp the second time with the same url, I'm getting the following error.

image.

Please let me know if there is a way to get it working or if we can have any changes on the FlurlCache to avoid throwing the error and ensure only one HttpClient is returned for a given url.

tmenier commented 10 months ago

@satishviswanathan I think you have 2 options to get this working:

  1. Do FlurlHttp.ConfigureClientForUrl... once at startup, or even from a static constructor in the service class. Then use the clientless pattern to make your calls, i.e. "https://api.com/some/path".SendAsync.... (Those calls will look to that static FlurlHttp object to get and reuse the configured client.)
  2. Keep things like you have them but use FlurlHttp.Clients.GetOrAdd (instead of ConfigureClientForUrl) to configure the client. This is guaranteed to create/configure the client at most once.
tmenier commented 10 months ago

Small update. As (previously) described above, IFlurlClientCache.Add had 2 overloads:

A sub-thread here made me realize a few things:

  1. The second one has a big advantage besides just chaining Add calls: returning the IFlurlClientCache makes DI registration (its primary use case) easier.
  2. The subtle differences between the overloads could be confusing, specifically with DI registration.

So, the first overload above will be dropped, and all but the first arg of the second one will be optional.

cremor commented 1 week ago

Is there a reason why IFlurlClientCache always requires a cache key (name parameter for Add and GetOrAdd)? If would be nice if we could use the default client-per-host caching strategy of the clientless model also with IFlurlClientCache by just providing a base URL.