[RFC] RestClientFactory for better DI support

[alexeyzimarev]

Issue

When registering the client instance, the last registered client will be injected, if the client is registered as AddSingleton<RestClient>. The only way to work around this is to use typed clients or wrappers.

It works similarly to AddHttpClient, except that AddHttpClient registered HttpClient as a transient dependency, and uses the client factory behind the scenes to pool the message handlers.

It would be nice to support registering named instances, where each named instance has its own options. Then, resolving a particular client instance would be through the factory. It's also similar to IHttpClientFactory behaviour in that sense. It should also be possible to register typed clients the same way as it's done with IHttpClientFactory, where the type name of the typed client is used as the options name.

What's unclear

• Should named RestClient instances be registered as singletons, or transient? Transient is more work (see the next point)
• Should we use IHttpClientFactory behind the scenes to delegate the handler pooling work? It only makes sense if we decide to support the transient scope

Based on the decision for the above points, it would be possible to decide on the implementation.

[kendallb]

I would suggest layering on top of IHttpClientFactory for this and letting it do all the hard work. One of the biggest advantages that I see with using transient HttpClient's grabbed from IHttpClientFactory, is that those are also expected to be transient and behind the scenes are using pooled HttpHandlers and have a life cycle of their own and are disposed of after a period of time (2 minutes by default I think) to avoid stale DNS issues.

That way you can then support transient RestClient's again using transient HttpClient's internally and let IHttpClientFactory do all the hard work.

One thing I am not clear on with IHttpClientFactory is how they handle cookies because the cookie container is always registered with the HttpClientHandler. I suspect (and would need to check the IHttpClientFactory implementation to be sure), that the cookie container is always set to a null value, or an empty container when a new HttpClient is requested from the factory, so that the user of the HttpClient can set it's own cookie container and it would not end up getting re-used the next time another request makes use of a pooled HttpClientHandler.

[kendallb]

Well that answers that. Don't use Cookies with HttpClient! LOL. Which means to support cookies correctly in RestClient and static HttpClient's (or using HttpClientFactory), the cookie handling has to be removed and handled at the request level with custom code.

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-6.0#cookies

Cookies
The pooled HttpMessageHandler instances results in CookieContainer objects being shared. Unanticipated CookieContainer object sharing often results in incorrect code. For apps that require cookies, consider either:

• Disabling automatic cookie handling
• Avoiding IHttpClientFactory

[alexeyzimarev]

Yes, I mentioned that before in another issue, don't remember which one. That's why the RestClient constructor that uses the provided HttpClient instead of creating its own will throw if the cookie container is not null in the options.

[kendallb]

If we use my PR to fix the cookie issues, we probably do not need to use HttpClientFactory and then maybe RestClientFactory is not that much use anymore?

#1795

Although I do suspect having RestClientFactory to implement a named (or typed) factory for materializing RestClient instances might be a good way to explain to those looking to use RestSharp that the clients really need to be single instanced on a per API basis.

[kendallb]

I have built my own factory and am now using it to test our code in development as I am done with the port (which took a week). I still think having the ability to use RestClient per request can help, if we used a HttpClient cache internally.

Would you be interested in me building one in another PR?

[alexeyzimarev]

If you can share the factory code somewhere, I am more than happy to have a look.

[kendallb]

Well my factory code is a factory for RestClient itself but I think the correct solution is an internal factory for HttpClient. Once my changes for cookies are accept upstream, then it really all boils down to managing HttpClient instances and reusing them across requests when the options match. At least that is the approach I have taken.

I could potentially implement my own HttpClient factory and use that to allocate instances to pass to RestClient, but it would be nice for that to simply be managed internally. The main problem I think is that while most options are simple to key off of, some are not. In my case we don’t use any of those options so I can make my keys relatively simple. The key stuff once cookies are gone is all the configuration of the underlying handler:

RestSharp/src/RestSharp/RestClient.cs

Line 132 in 17a3532

handler.Credentials = Options.Credentials;

The problem ones are the proxy, certificate callback and certificates themselves. You would need a way to generate a signature for those to keep them separate. For the proxy I am able to generate a sting to represent the proxy config given how we use it, and for the certificate callback we only use it to disable SSL for some services (mostly internal testing stuff) so I just made that a Boolean. And we don’t use client certificates so no issue there. Maybe there is a way to use the certificate thumbprints for sort that out?

You can see why HttpClientFactory allows for typed handlers as that makes it a lot simple that is for sure. I can’t use typed handlers as we have a massive client class called DocumentLoaded that is used all over the place in older code that handles requests to any url, so that one I need to generate a named key to keep them separate :(

And for my case we are currently still on .net 4.8 using Autofac so we can’t really use HttpClientFactory anyway I don’t think (and I would still need to generate a similar key to get named instances if I did want to use it).

[kendallb]

Here is what we are currently using for our client factory. But as explained above, we cannot support some stuff so I just turned it off so if we do try to use it with those set in RestClientOptions, it would fail. Not entirely sure there is a clean way to implement keying off those options.

We also assume that the appropriate serializer set on the client is never changed for that client instance so we don't vary that. I don't know of a clean way to key on that since it's a function callback and we have quite a few spots where we need to use custom JSON serialization settings for certain REST services we connect to. That is one reason I think the seralizer really should not be part of the client at all, if we are expected to use them as singletons - it should be set on the request so you could theoretically have a different serializer for different requests to the same REST endpoint. Or more importantly if you happened to share an instance across two different endpoints (like in our DocumentLoader code), it won't cause an issue. But in our code our DocumentLoader is never used for REST services at all, it's just for HTML scraping stuff so the serializer does not matter. I probably should re-write that to live directly on top of HttpClient anyway I think.

using System;
using System.Collections.Generic;
using System.Net;
using System.Text;
using RestSharp;
using RestSharp.Serializers;
using RestSharp.Serializers.NewtonsoftJson;
using RestSharp.Serializers.Xml;

namespace AMain.RestClientHelper
{
    public static class RestClientFactory
    {
        /// <summary>
        /// Dictionary of all static client instances based on the client API base URL
        /// </summary>
        private static readonly Dictionary<string, RestClient> _clientCache = new();

        /// <summary>
        /// Get a client bound to a specific URL that uses only JSON for serialization
        /// </summary>
        /// <param name="cachePrefix">Cache prefix to use for the client cache</param>
        /// <param name="baseUrl">The client base URL</param>
        /// <param name="disableSslChecks">True to disable SSL certificate checks</param>
        /// <returns>RestClient instance to use</returns>
        public static RestClient GetJsonClient(
            string cachePrefix,
            string baseUrl,
            bool disableSslChecks = false)
        {
            return GetClient(cachePrefix, new RestClientOptions(baseUrl), disableSslChecks, () => new JsonNetSerializer());
        }

        /// <summary>
        /// Get a client bound to a specific URL that uses only XML for serialization
        /// </summary>
        /// <param name="cachePrefix">Cache prefix to use for the client cache</param>
        /// <param name="baseUrl">The client base URL</param>
        /// <param name="disableSslChecks">True to disable SSL certificate checks</param>
        /// <returns>RestClient instance to use</returns>
        public static RestClient GetXmlClient(
            string cachePrefix,
            string baseUrl,
            bool disableSslChecks = false)
        {
            return GetClient(cachePrefix, new RestClientOptions(baseUrl), disableSslChecks, () => new XmlRestSerializer());
        }

        /// <summary>
        /// Get a client bound to a specific set of client options. We store static instances of RestClient for each
        /// base URL and related options as we do not want to have an instance per request, and we can cache them statically
        /// and reuse them over and over as they are thread safe provided we fully configure the RestClient in here.
        /// </summary>
        /// <param name="cachePrefix">Cache prefix to use for the client cache</param>
        /// <param name="options">The client options for this client</param>
        /// <param name="disableSslChecks">True to disable SSL certificate checks</param>
        /// <param name="serializerFactory">Optional custom serializer factory to use for requests and responses</param>
        /// <returns>RestClient instance to use</returns>
        public static RestClient GetClient(
            string cachePrefix,
            RestClientOptions options,
            bool disableSslChecks = false,
            Func<IRestSerializer> serializerFactory = null)
        {
            // Bail if we see unsupported features
            if (options.ConfigureMessageHandler != null) {
                throw new ArgumentException("RestClientFactory does not support ConfigureMessageHandler!", nameof(RestClientOptions));
            }
            if (options.Credentials != null || options.UseDefaultCredentials) {
                throw new ArgumentException("RestClientFactory does not support Credentials!", nameof(RestClientOptions));
            }
            if (options.ClientCertificates != null) {
                throw new ArgumentException("RestClientFactory does not support ClientCertificates!", nameof(RestClientOptions));
            }
            if (options.RemoteCertificateValidationCallback != null) {
                throw new ArgumentException("RestClientFactory does not support RemoteCertificateValidationCallback!", nameof(RestClientOptions));
            }
            if (options.Timeout > 0) {
                throw new ArgumentException("RestClientFactory does not support Timeout! Please set it at the request level", nameof(RestClientOptions));
            }

            // Determine the string to use for the proxy if passed in
            var proxyString = "";
            if (options.Proxy is WebProxy proxy) {
                if (proxy.Credentials != null) {
                    var c = proxy.Credentials as NetworkCredential;
                    proxyString = $"http://{c.UserName}:{c.Password}@{proxy.Address.Authority}/";
                }
                proxyString = proxy.Address.AbsoluteUri;
            }

            // Build a key to represent the client in the cache
            var sbKey = new StringBuilder()
                .Append(cachePrefix).Append(':')
                .Append(options.BaseUrl).Append(':')
                .Append(disableSslChecks).Append(':')
                .Append(options.DisableCharset).Append(':')
                .Append(options.AutomaticDecompression).Append(':')
                .Append(options.FollowRedirects).Append(':')
                .Append(options.MaxRedirects).Append(':')
                .Append(proxyString).Append(':')
                .Append(options.Encoding).Append(':')
                .Append(options.PreAuthenticate).Append(':')
                .Append(options.Expect100Continue);
            var key = sbKey.ToString();

            // If we have an existing instance of this client, return it outside of the lock
            if (_clientCache.TryGetValue(key, out var client)) {
                return client;
            }

            // Create a new instance, but make sure only one thread can create it
            lock (_clientCache) {
                // Try again to get it, as it might have just been created by another thread
                if (_clientCache.TryGetValue(key, out client)) {
                    return client;
                }

                // Disable the SSL certificate checks if needed
                if (disableSslChecks) {
                    options.RemoteCertificateValidationCallback = (_, _, _, _) => true;
                }

                // Set up the client with an infinite timeout so the request timeout will be the one that always matters
                options.Timeout = int.MaxValue;

                // Create the client and cache it
                client = new RestClient(options);

                // Set up the single serializer we want to use for this client if desired. If this is passed in as null,
                // it will use the defaults which is both the XML and Newtonsoft.JSON serializers. We do not use the
                // System.Text.Json one as it has a bunch of issues with our code.
                if (serializerFactory != null) {
                    client.UseOnlySerializer(serializerFactory);
                } else {
                    client.UseSerializer<JsonNetSerializer>();
                }

                _clientCache.Add(key, client);
                return client;
            }
        }
    }
}

[alexeyzimarev]

So, basically, you don't support properties that are reference types (functions, objects, etc), right?

My very first attempt to move to HttpClient has the options record that only had value types. It allowed easier equality, so it was possible to detect if the options record is different, so I can detect the need to create another client instance. However, HttpClient has some complex types in its properties, so it didn't work. I don't want to throw an "unsupported exception".

I will keep looking to integrate with IHttpClientFactory and named options using MS DI.

[kendallb]

Yeah that’s the problem with HttpClient. It has too much configured on it (even the handler does too). The problem I see with IHttpClientFactory is that it’s not easy to use outside of .net core is it?

[kendallb]

My main question with HttpClientFactory is whether there is a way to access the underlying handler as it’s configured when you create HttpClient and it’s not configured if someone passes one in. As far as I can tell once HttpClient is created you cannot access the underlying handler and configure it?

RestSharp/src/RestSharp/RestClient.cs

Line 132 in 17a3532

handler.Credentials = Options.Credentials;

so even with HttpClientFactory, you have to request clients by name or type, but I don’t see how you configured the underlying handler in the examples I have seen? What am I missing?

[kendallb]

Ok yeah, so all the HttpHandler stuff is configured with the ConfigurePrimaryHttpMessageHandler extension. Which is used to create a new handler, configure it and return it. Internally I imagine it is pooling these handlers and re-using them only within the confines of a particular named or typed HttpClient, otherwise they would be configured wrong:

https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.httpclientbuilderextensions.configureprimaryhttpmessagehandler?view=dotnet-plat-ext-6.0

So I don't see any practical way to combine that with RestSharp, since it's pretty much required that you configure it all as part of setting up the DI for your application. Which pretty much means you can't really set up RestClient to be configured that way, or it also needs to be set up and configured by DI also. I personally think this was a really bad design choice on Microsoft's part to tie this so heavily to DI. Probably why this guy did this:

https://github.com/uhaciogullari/HttpClientFactoryLite

Because if you have to rely on DI to use HttpClientFactory, it's not a useful pattern for a library like RestSharp, or any library that depends on RestSharp (like say EasyPost) because the opinions on how to configure it are then enforced on the client library also. Ie: you cannot just new up a HttpClientFactory inside RestSharp, and hence you can't just new up a RestClient and have it do the right things in a transient scope.

At the end of the day though, the critical part is the pooling, reuse and limited lifetime of HttpClientHandler internally as that is where all the problems stem from, and what we need to be reusing. Perhaps the best solution if there is going to be some kind HttpClientHandler pooling inside of RestSharp, similar to what HttpClientFactory does so for folks who do just want to new up a RestClient can do so, and everything works correctly with transient scope RestClient's. And developers who are using HttpClientFactory can still use RestSharp by creating their own opinionated instances of HttpClient and passing it to RestClient to use.

[alexeyzimarev]

That's exactly my concern about the HTTP client factory, as all of it is heavily container-dependant. It simply won't work if you don't accept the factory itself as a dependency. For example, creating a named client instance would require you to resolve the client using the factory as a dependency, providing the client name in the call.

I'm afraid that managing the pool of handlers inside RestSharp would increase the complexity significantly.

This code is what I am talking about: https://github.com/uhaciogullari/HttpClientFactoryLite/tree/hcf-lite/src/HttpClientFactory/Http/src

It's also hard (you already encountered it) to ensure the uniqueness of handlers by looking at the options object, as some of the options are properties of reference types.