ASP.NET Best Practices?

[elibroftw]

Is it better to create a new postmark client for each controller, or should we be using a Singleton or something like that and create only one client?

For reference, Microsoft tutorials for MongoDB creates a service for a collection and uses that service/singleton to insert/get.

[elibroftw]

// Program.cs
builder.Services.AddSingleton(new PostmarkClient(builder.Configuration["PostmarkServerToken"]));
// MyController.cs
    public MyController(PostmarkClient postmarkClient) {
        // Initialize Postmark client
        _postmarkClient = postmarkClient;
}

[MariuszTrybus]

Postmark-dotnet internally uses a static instance of HttpClient that is shared by all postmark clients - you can use either approach.

[rjgotten]

Postmark-dotnet internally uses a static instance of HttpClient that is shared by all postmark clients

This is an anti-pattern. For ASP.NET Core applications HttpClient is meant to be used from the service collection with AddHttpClient which registers a transient factory dependency that will use the framework implementation of IHttpClientFactory to create clients.

The clients are a thin layer on top of a message handler pool that handles the actual HTTP connections. When a new HttpClient is created, the pool is checked for available compatible handlers that haven't expired yet. If any are available they are reconnected and reused. When an HttpClient is disposed (at the end of a service scope's lifetime) its handler is returned to the pool where it remains until it becomes stale.

The Http clients themselves are basically just thin wrappers to freely create/destroy with transient scope. All the heavy lifting is done in the message handlers whose lifetime is managed by the pool internal to the IHttpClientFactory implementation type.

If instead you use a static HttpClient then the handler is never allowed to return to the pool for as long as the application is running. This also means the handler, once stale, can never close and be refreshed. Among other things this means e.g. underlying DNS changes never propagate. It can lead to incredibly hard to diagnose 'random' network failures of a non-transient nature - that are instantly resolved by killing and restarting an application only to return at another 'random' future moment.

If this library is using an internal static field holding an HttpClient, then it is 'doing it wrong,' irrefutably.

[elibroftw]

Thank you @rjgotten . As someone who is fairly newish at C# and .net, I only knew how to create http clients for asp.net but could never give such a fabulous answer on why it's done so which is why I asked my question to begin with.

[rjgotten]

@elibroftw Don't mention it. I see this going wrong a lot - mainly because it used to be the other way around. For .NET Framework, the advice was to create HttpClient instances once, and reuse them as much as possible. Because they were top-heavy back then; didn't have the separation with all the heavy stuff living in pooled message handlers under the hood, but were each self-contained, handling the whole thing front-to-back.

That said, I just had a slightly deeper look and... yikes. The way the SimpleHttpClient wrapper is implemented is using a finalizer rather than following the basic Dispose pattern. In .NET Core 5 and up finalizers are not guaranteed to run upon process termination. Which has some potentially nasty repercussions for network resources not being freed; network connections not being gracefully closed; etc.

This type of thing really needs to be Disposed correctly and should be using a System.AppDomain.ProcessExit handler to call the Dispose on the HttpClient to ensure that that happens before the process actually fully exits.

Normally you wouldn't have to worry about that because IServiceProvider calls Dispose on anything that is IDisposable when it itself is disposed. And it is disposed as part of the orderly shutdown. But if you start involving HttpClient instances that live on static fields ...