Proper use of WebServiceClient in .NET Core
nikolic-bojan opened this issue · 17 comments
Hi,
I would like to have a sample of proper use of WebServiceClient in .NET Core.
If we use it with using (client = new WebServiceClient()), that is not good as we will have a bunch of connections opened too long, not doing anything.
You suggest in documentation to reuse it. OK, usually it is done with AddSingleton with DI, but there is an issue with DNS refreshing. In "old" Framework, you could deal with that with DnsRefreshTimeout. Also ConnectionLeaseTimeout etc.
There is no such thinsg in .NET Core. Therefore, they introduced IHttpClientFactory in 2.1 and it is dealing with those issues.
Now, how I do I achieve having a proper implementation in .NET Core?
BR,
Bojan
If you want more control over various timeouts and pooling options, I would suggest passing a SocketsHttpHandler to WebServiceClient using the optional httpMessageHandler parameter.
Hi @oschwald ,
Thank you for the suggestion, but I can only find httpMessageHandler optional parameter in internal constructor of WebServiceClient.
Am I missing something?
BR,
Bojan
Ah, right. I was thinking of our minFraud client. We can expose that in the public constructor in the future.
@oschwald Yes, maybe that would help.
What we did is that we made an implementation for .NET Core of the client (few months back) that is working fine with IHttpClientFactory. We reused your model though.
Trouble we encountered recently when we updated to 3.1.0 is that you introduced Network convertor that broke our service, as we didn't know this was added and standard Newtonsoft serialization broke.
TBH, I am not happy with this convertor and it kind of feels like 3.1.0 is a breaking change
compared to 3.0.x, but I guess it is up to us to be more careful since we have custom implementation and reuse your model.
Is there a particular reason why you are concerned with DNS refreshing? With our infrastructure, that shouldn't generally be an issue.
DNS refreshing was not that much of a concern, I just wanted to do it right for Core implementation. I understand you need to make it work for multiple frameworks and having one NuGet is easier than having several to support best different frameworks.
With our implementation we have much greater control regarding building HttpClient. We can setup in more details with DefaultRequestHeaders, we can add our custom DelegatingHandlers, resililence with Polly extension methods...
Our implementation is still encapsulated as yours could be, but all these things regarding configuring HttpClient related things that I mentioned could still be added by AddHttpClient extension method, available since Core 2.1.
Again, I understand that having several NuGets just to split some stuff and make specific NuGet for Core is a bit tedious work and will require more effort and can maybe lead to more maintenance, but you can't blame man for asking :)
In #116, I added the HttpMessageHandler parameter to the public constructor.
That said, supporting IHttpClientFactory makes sense to me. A PR for that would likely be accepted as long as it didn't affect Framework or older Core users and had appropriate tests.
Thank you @oschwald !
I will also try to create a branch for IHttpClientFacotry support and will get back to you with PR in several days. If you have some suggestion on that up front, please let me know.
@oschwald
Hi, this is initial work on a support for injecting HttpClient.
https://github.com/nikolic-bojan/GeoIP2-dotnet/tree/feature/httpclientfactory
You should be able to check what I added.
This is how I setup in Startup.cs
services.AddControllers().AddNewtonsoftJson(options =>
{
options.SerializerSettings.Converters.Add(new NetworkConverter());
});
services.Configure<WebServiceClientOptions>(options =>
{
options.AccountId = 123456;
options.LicenseKey = "test";
options.Timeout = 3000;
options.Host = "geoip.maxmind.com";
options.Locales = null;
});
services.AddHttpClient<WebServiceClient>();I haven't configured HttpClient as you do this internally in code, so that needs to go with Options as recommended approach, not by configuring in Startup.cs, like here https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-3.1#named-clients
I had to copy/paste NetworkConverter if I wanted just to output response in my controller action. TBH, not happy with having that custom convertor. Hope it will be gone in v4 if possible.
This is my controller
[ApiController]
[Route("[controller]")]
public class MaxMindController : ControllerBase
{
private readonly WebServiceClient _maxMindClient;
public MaxMindController(WebServiceClient maxMindClient)
{
_maxMindClient = maxMindClient;
}
[HttpGet]
public async Task<CountryResponse> Get()
{
var location = await _maxMindClient.CountryAsync();
return location;
}
}Since WebServiceClient is considered a typed HttpClient, it is injected and in its constructor both HttpClient and WebServiceClientOptions are injected.
Please let me know what do you think about the code, what could/should be improved etc.
It looks like a reasonable start. I wonder if we can make the configuration a bit easier to use, but it isn't too bad.
We will need tests and examples in the README.md.
Re: JSON serialization, I suspect we will be switching to System.Text.Json in the future, once it is sufficient for our needs. It sounds like you need to serialize the objects back to JSON for storage or something. I wonder if your needs would be better supported by the addition of ToJson methods (or similar) to the response objects so that you don't need to do the serialization yourself or keep up with any custom converters (or other serializer settings) that may be required. We provide similar functionality for other languages.
I changed few things as you suggested:
- AsyncClient doesn't have code duplicate in constructor
- WebServiceClient constructor now uses IOptions
- Copy/pasted tests to use for Core with a WebServiceClient created with new constructor. Not to subtle, I guess.
Only thing left is some sample in Readme.md, if you are satisfied with the code and tests.
Configuration could be easier and it could be done in Startup.cs with HttpClient configuration, but we have Locales that do not fit there at all. This should be easy as Options can be configured to point to a section in appsettings.json, so reading is a breeze.
Sorry, one thing regarding tests - Sync Tests were only for !NETCOREAPP1_1
I extended that to exclude also Core 2.1 and Core 3.0 because it was strange having those tests when using new HttpClient injection.
On the other hand, there are a lot of places in code where you have !NET_STANDARD1_4, but Standard 2.x was not also excluded...
So, not sure what to do with those really because I tried to touch as little code I had to.
This is looking good! Do you mind making a PR? This will make it easier for me to provide comments.
Given that the Sync code is usable on .NET Standard 2+, we should probably leave those tests live. I agree that it is weird to have the sync methods when injecting the HttpClient, but we are kind of stuck with them, at least until a major release.
PR submitted!
Haven't made any change regarding documentation. It would be best if you decide on how it should be changed or to at least give me structure with titles. I can fill in the details.
Thanks! Re: documentation, I think primarily an example of how to use it with ASP.NET, e.g., what to put in Startup.cs, etc. I think it could go under "Web Service Usage" and be called "ASP.NET Usage" or something like that.
Did those small changes and also added documentation based on your suggestion.