search

avivbiton / BlizzardApiReader

.Net Core library to handle Blizzard public APIs
MIT License
35 stars 36 forks source link

Changed how ApiWebClient works with HttpClient #45

Closed oliverkovac closed 6 years ago

oliverkovac commented 6 years ago

I've changed ApiWebClient to reuse the HttpClient (see i.e. https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/) which to my surprise considerably increased the performance of sending multiple requests in a row.

Given these changes I had to modify the structure of other classes, such as moving URLs to Configuration and letting it create them based on configured regions, etc. Also ApiReader now only sends the path and query to the WebClient which appends them to the API domain.

I've also added a few tests, mostly useless ones just to increase coverage. Also I've tested a TravisCI integration on my fork, which seems to work well, so if interested feel free to use it, just gotta integrate this repository in TravisCI and edit a few paths which at the moment refer to the fork.

I know this is quite big and I should probably split it into smaller requests, but initially I just planned to toy around and experiment a bit more in my fork and ended up finding the thing about HttpClient, so I had to let you guys know!

avivbiton commented 6 years ago

Everything looks good! can you just explain the usage of the ServicePointManager and why it is necessary? (I am not familiar with this class)

oliverkovac commented 6 years ago

Oh yes, I was digging around a bit and found https://github.com/dotnet/corefx/issues/11224. Long story short, if you plan to reuse the HttpClient for a longer time instead of in a using block, the one bad thing that can happen is, that the target domain IP address changes, but as the HttpClient doesn't do DNS lookup with each request, it would keep sending requests to the old, possibly no longer viable IP address. I don't know if this would ever happen with Blizzard, but better safe than sorry - provided the ServicePointManager thing I put there actually works. It should now refresh the IP addresses from DNS every minute, while the default configuration seems to be never, that's why I had to set it. The only thing I'm unsure of is whether its enough to do this for the domain as it is now, or for each endpoint separately. Ive seen multiple articles dealing with this problem and while some only applied it for the domain (i.e. only to https://eu.api.blizzard.com) others applied the same thing to each endpoint separately. I'm actually afraid its the latter, so I might have rework it to set it on all these endpoints dynamically. I'll try to look around a bit more and do some testing.

avivbiton commented 6 years ago

I may end up testing when i get the time. Is it suppose to throw an error when that happens? Anyway, good job, merging.

oliverkovac commented 6 years ago

I think the only thing that would happen in case Blizzard servers changed IPs right while someone using the library would have open connections would be that the following requests would be unsuccessful, because the old IP address wouldn't return and hence they'd time out. It's a really cumbersome thing to even replicate or test, and who knows if/how often Blizzard changes the underlying IP addresses of their domains, so it's more like a "keep it in there just to be sure and hope for the best" thing.

I've opened a new PR with a better solution for the connection lifetimes. Also when you have time, remember to make an account and link this repo on https://travis-ci.com/ and change the url of the Build status image in the readme file, because right now it points to my fork.