Conversation
| this FluentEmailServicesBuilder builder, | ||
| string serverToken) | ||
| { | ||
| builder.Services.TryAdd(ServiceDescriptor.Scoped<ISender>(_ => new MailPaceSender(serverToken))); |
There was a problem hiding this comment.
What was the reason to not bind this in Singleton scope? If there are a large number of concurrent requests couldn't you get resource exhaustion due to all the HttpClients that are created in MailPaceSender's constructor?
Microsoft indicates either HttpClient should be created in singleton scope or use IHttpClientFactory. https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines
There was a problem hiding this comment.
You are right! Singleton makes more sense here.
There was a problem hiding this comment.
The one caveat with singleton on this class is not being able to set the PooledConnectionLifetime on the internal HttpClient itself. Binding in Singleton scope would help with resource exhaustion but there could be a potential issue with long lived connections. Changes to DNS records may not be picked up. It might be better to manage the HttpClient lifecycle inside MailGunSender and MailPaceSender and set the PooledConnectionLifetime to some reasonable default. Then it won't matter how ISender is bound.
There was a problem hiding this comment.
Would passing in IHttpClientFactory be a better option? Let the DI container handle HttpClient lifetimes?
There was a problem hiding this comment.
Yes I think that should work too.
I had a play around with this for MailGunSender in #382
2 participants
Fixes #377