5
\$\begingroup\$

I initially posted this on Stackoverflow but was recommended to try here instead. I'm looking for feedback on why the implementation of my ClientFactory is probably bad, from a Dependency Injection-point of view. Or if it is actually okay to use in production?

My code below works. I know how to implement a named/typed client. Yes, I have read the docs on how to implement resilient HTTP requests and yes, I have read about socket exhaustion here and here

I'm creating an REST API that will communicate with various other APIs in it's "backend". The APIs it will communicate with will alot of the time be the same API (same contract) but on a different instance/host. The hosts are not known to me specifically, but are retrieved from a database based upon a request is initiated for that specific system. I do not wish to create a new HttpClient for every individual request so I would like to re-use my HttpClients for some time after it's been created for subsequent requests to the same system to avoid socket exhaustion.

To remedy this I have tried a few different approaches, but since I'm feeling unsure on the inner workings of DI I'm not sure my implementations makes sense with the concept of Dependency Injection. The DI-concept just doesn't come naturally to me at this point. Here's my examples:

ClientFactory:

I've created an Interface and ClientFactory for each API that I will use. The ClientFactory.Create() takes a settings-parameter that sets hostname and authorization headers for the specific server I need to connect to for this specific request. It looks a bit like this:

MyApiClientSettings.cs:

namespace MyAPI.Domain.Services.ConnectionHandler
{
    public interface IMyApiClientSettings
    {
        string Server { get; set; }
        string Token { get; set; }
    }

        public class MyApiClientSettings : IMyApiClientSettings
    {
        public string Server { get; set; }
        public string Token { get; set; }
    }
}

MyApiClientFactory.cs:

ClientFactory sets BaseAddress and auth-header based on supplied settings

namespace MyAPI.Domain.Services.ConnectionHandler
{
        public interface IMyApiClientFactory
    {
        MyApiClient CreateMyApiClient(IMyApiClientSettings settings);
    }

    public class MyApiClientFactory : IMyApiClientFactory
    {
        private const string MyApiEndpointUrl = "https://{0}/MyApi/";

        public MyApiClient CreateMyApiClient(IMyApiClientSettings settings)
        {
            var myApiClient = new MyApiClient();
            myApiClient.BaseAddress = new Uri(string.Format(MyApiEndpointUrl, settings.Server));
            myApiClient.DefaultRequestHeaders.Add("Accept", "application/json");
            myApiClient.DefaultRequestHeaders.Add("User-Agent", "My-API-Consumer");
            myApiClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue($"Bearer", $"{settings.Token}");

            return myApiClient;
        }
    }
}

MyApiClient.cs:

The MyApiClient that is returned from ClientFactory.

namespace MyAPI.Services.ConnectionHandler
{
    public class MyApiClient : System.Net.Http.HttpClient
    {
        public async Task<UserList> GetUsersListAsync()
        {
                var response = await this.GetAsync($"v1/users/");
                response.EnsureSuccessStatusCode();
                var content = await response.Content.ReadAsStreamAsync();
                var result = DeserializeObjectAsync<UserList>(content);
                return result;
        }
        public async Task<User> GetUsersByIdAsync(Guid id)
        {
                var response = await this.GetAsync($"v1/users/{id}");
                response.EnsureSuccessStatusCode();
                var content = await response.Content.ReadAsStreamAsync();
                var result = DeserializeObjectAsync<User>(content);
                return result;
        }
    }
}

Used like this: The ClientFactory is injected into a service using dependecy injection. The WorkerService builds the settings for MyApiClient and gets the hosts IP (/BaseAddress) by quering GetServerIPByToken(apiToken).

namespace MyAPI.Services
{
    public class MyService
    {
        private readonly MyApiClientFactory _myApiClientFactory; //Injected from Startup.cs via "services.AddSingleton<MyApiClientFactory>();"

        public MyService(MyApiClientFactory myApiClientFactory)
        {
        _myApiClientFactory = myApiClientFactory;
        }

        public void DoStuff(string apiToken)
        {
        var settings = new MyApiClientSettings 
        {
        Token = apiToken,
        Server = GetServerIPByToken(apiToken)
        };

        var myApiClient = _myApiClientFactory.Create(settings);
        var result = myApiClient.GetUsersListAsync();
        /* Do stuff with result - Removed for the sake of brevity */
        }
    }
}

This way I can dynamically create my API clients for various hosts depending on the requests I recieve. But to do so I inject a Singleton ClientFactory to every service that might need it. Add a couple of worker services, and those worker services then creates its own instaces of MyApiClient by calling _myApiClientFactory.Create(settings); which seems potentially wasteful.

So I added a "ConnectionHandler" that creates the MyApiClient-clients, and adds them to a dictonary. The ConnectionHandler creates/reconnects clients and adds them to the dict, disposes of clients and removes unused or failed clients from the dictonary. This way I inject the MyApiClientFactory only to my ConnectionHandler and instead injects my ConnectionHandler to all services, wich then use the exposed clientDictonary, like this:

ConnectionService.cs: Initialize clients to be used by services, handle disconnects/reconnects and dispose faulty or unused clients.

namespace MyAPI.Services.ConnectionHandler
{  
  public class ConnectionService
    {
        private readonly MyApiClientFactory _myApiClientFactory; //Injected from Startup.cs via "services.AddSingleton<MyApiClientFactory>();"

        public ConnectionService(MyApiClientFactory myApiClientFactory)
        {
        _myApiClientFactory = myApiClientFactory;
        }

        /// <summary>Dictionary containing active apiClients.
        /// <para>TKey - apiToken. TValue - active MyApiClient.</para>
        /// </summary>
        public Dictionary<string, MyApiClient> clientDict = new Dictionary<string, MyApiClient>();

        public void Initialize(string apiToken, bool reconnect = false)
        {
         var settings = new MyApiClientSettings 
         {
         Token = apiToken,
         Server = GetServerIPByToken(apiToken)
         };

         var myApiClient = _myApiClientFactory.Create(settings);
         clientDict.Add(apiToken, myApiClient);
        }
        /* Handle reconnects, disconnects, dispose of clients that have been unused for 5+ minutes etc */
    }
}

MyService.cs updated to: Use existing MyApiClient or call Initialize if there is no existing client.

namespace MyAPI.Services
{
    public class MyService
    {
        private readonly ConnectionService _connectionHandler; //Injected from Startup.cs via "services.AddSingleton<ConnectionService>();"

        public MyService(ConnectionService connectionHandler)
        {
            _connectionHandler = connectionHandler;
        }

        public void DoStuff(string apiToken)
        {
            if (!_connectionHandler.clientDict.TryGetValue(apiToken, out var myApiClient))
            {
                _connectionHandler.Initialize(apiToken);
            }
            _connectionHandler.clientDict.TryGetValue(apiToken, out myApiClient);
            var result = myApiClient.GetUsersListAsync();
            /* Handle connection errors etc */
        }
    }
}

I personally like this approach. It feels natural for me to use and fairly easy to scale. I am however in-experienced when it comes to Dependecy Injection so I suspect this is full of anti-patterns, bad practices and probably stuff that violates the DI-concept.


From my understanding, "the correct way" to do the above would be to skip most of the code and ClientFactory, and add MyApiClientvia Dependency Injection and then supply a httpClient for each request. Something similar to this:

The right way (ish):

MyApiClient.cs:

namespace MyAPI.Services
{
    public class MyApiClient
    {
        public async Task<UserList> GetUsersListAsync(HttpClient httpClient)
        {
                var response = await httpClient.GetAsync($"v1/users/");
                response.EnsureSuccessStatusCode();
                var content = await response.Content.ReadAsStreamAsync();
                var result = DeserializeObjectAsync<SystemApplicationList>(content);
                return result;
        }
        public async Task<User> GetUsersByIdAsync(HttpClient httpClient, Guid id)
        {
                var response = await httpClient.GetAsync($"v1/users/{id}");
                response.EnsureSuccessStatusCode();
                var content = await response.Content.ReadAsStreamAsync();
                var result = DeserializeObjectAsync<SystemApplicationList>
        }
    }
}

MyService.cs:

namespace MyAPI.Services
{
    public class MyService
    {
        private readonly MyApiClient _myApiClient //Injected from Startup.cs via "services.AddHttpClient<MyApiClient>();"

        public MyService(MyApiClient myApiClient)
        {
        _myApiClient = myApiClient;
        }

        public void DoStuff(string apiToken)
        {
            var httpClient = new HttpClient();
            httpClient.BaseAddress = new Uri(GetServerIPByToken(apiToken));
            httpClient.DefaultRequestHeaders.Add("Accept", "application/json");
            httpClient.DefaultRequestHeaders.Add("User-Agent", "My-API-Consumer");
            httpClient.DefaultRequestHeaders.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue($"Bearer", $"{apiToken}");


        var result = _myApiClient.GetUsersListAsync(httpClient);
        /* Do stuff with result - Removed for the sake of brevity */
        }
    }
}

What I do not like or maybe do not understand fully, is that in this aproach every call to DoStuff() creates a new instance of a httpClient. This seems wasteful to me. Sure I could create and re-use the httpClient in a similar way that I did in my ConnectionHandler - but then I'd rather prefer my implementation of Interfaces & Factories instead of passing a client as a parameter for every action. But I suspect the reason for my preference is based on some misunderstanding of how DI works or is supposed to be used.

\$\endgroup\$
  • 4
    \$\begingroup\$ Welcome to code review. The question does reflect good effort on your part, but some parts of the question seem to be vague. If the title indicated a little more about what the code does it might be better, also comment like /* .... */ indicate some code is being hidden and name spaces like foobar make the question seem hypothetical which would make it off-topic for code review. \$\endgroup\$ – pacmaninbw Nov 15 at 16:16
  • 4
    \$\begingroup\$ Thanks @pacmaninbw - I've updated the title and namespaces and I've updated or removed some of the comments. My initial choice of namespaces and comments was an attempt to make an already long post shorter for the sake of brevity. I hope its better now. \$\endgroup\$ – Rasmus Westerlundh Nov 15 at 17:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.