RegistryManager->HttpClientHelper creates a non-static HttClient wich is an antipattern

[tymtam2]

Context

HttpClientHelper, created every time an instance of RegistryManager is created, creates a non-static HttpClient which is considered an antipattern.

HttpClients documenation states:

HttpClient is intended to be instantiated once and reused throughout the life of an application. In .NET Core and .NET 5+, HttpClient pools connections inside the handler instance and reuses a connection across multiple requests. If you instantiate an HttpClient class for every request, the number of sockets available under heavy loads will be exhausted. This exhaustion will result in SocketException errors.

Singleton vs on demand

I could not find any documentation that makes it clear if RegistryManager should be used as a singleton (HttpClient pattern) or is using var registryManager = ... recommended. (If having a local variable on demand is the preferred way then it results in multiple HttpClients and this doesn't seem correct). I would have created a separate issue for the documentation to add this clarification but I couldn't find an appropriate documentation page.

Code sample exhibiting the issue

The example at https://github.com/Azure-Samples/azure-iot-samples-csharp/blob/main/iot-hub/Samples/service/RegistryManagerSample/RegistryManagerSample.cs
shows having a local RegistryManager. RegistryManagerSample is used once in the application but arguably maybe RegistryManager should be static anyway to promote correct techniques. (If having a local variable on demand is the preferred way then it results in multiple HttpClients and this doesn't seem correct).

using RegistryManager registryManager = RegistryManager
                .CreateFromConnectionString(_parameters.IoTHubConnectionString);

            try
            {
                await CreateDeviceHierarchyAsync(registryManager);

                await AddDeviceWithSelfSignedCertificateAsync(registryManager);

                await AddDeviceWithCertificateAuthorityAuthenticationAsync(registryManager);

                await EnumerateTwinsAsync(registryManager);
            }

[drwill-ms]

Hi @tymtam2 and thank you for reporting the issue.

I agree this could be better and we'll take a look at whether making the HttpClient instances static would be a good change.

I suspect we expect most users to create an instance of RegistryManager for their service application and use it for the duration of the lifetime of that app, but there are cases where that isn't possible (like Azure Function apps). This class should work just as well in either usage scenario, so we'll do our best to resolve the problem.

[drwill-ms]

I've taken a deeper look at the code and found that the HttpClient instances are created with a specific proxy and base URI as specified in the registry manager connection information. That means if we made them static then a process couldn't have multiple instances of RegistryManager working against different IoT hubs or, and as unlikely as this seems, with different proxy settings. I know we could avoid setting the base URI but it looks like we do that to set connection lease timeouts to fix this issue. We'd need to investigate more to see if we can do that without specifying the base URI.

So, that said, unless I have something wrong, I think it would be best for a service application to create and cache the registry manager instance. I think the most important thing is that RegistryManager doesn't create a new HttpClient for each request, so as long as the SDK instance stays around, the client application should be fine WRT the HttpClient anti-pattern.

Thoughts @tymtam2?

[tymtam2]

@drwill-ms Thanks for looking into this.

Not many thoughts :) We'll move to using a static RegistryManager.

Should I raise a separate issue for the ServiceClient which uses HttpClientHelper in the same manner as RegistryManager?

[drwill-ms]

I've added the same documentation to the ServiceClient class, and others.

FWIW, your registry manager instance doesn't need to be static, but it should stick around between operations calls. If you just avoid disposing and reinitializing a new one, you should be fine.

[tymtam2]

Good point, I was using 'static' as a shortcut to ~"not initialised->disposed often (e.g. per request)".