Make HTTP components configurable

[yaakov-h]

Users can provide one or two new factory functions to SteamConfiguration:

• A HttpMessageHandler factory function, which is the inner part of the pipeline that actually executes HTTP requests, and
• A HttpClient factory function, which is a nice wrapper class that the rest of the .NET HTTP stack is built around.

Users can use these two injection points to, for example:

• Configure credentials
• Configure proxy settings
• Do something instead of HTTP requests
• Inspect and log outgoing requests
• Inspect and log incoming responses
• Mutate requests and responses (though any errors caused from this are their own fault)
• Use a single underlying HTTP connection pool to increase performance
• Use a single underlying HTTP connection pool to avoid connection exhaustion

Note that this only affects WebAPI, it has no bearing on Websocket CM connections. We could expose ClientWebSocketOptions some day, but that's another PR for another issue, if/when it's ever needed.

This resolves #450 and #541.

For now I've simply attached them to SteamConfiguration, but as the configuration gets bigger we might want to split it up - for example, these two factory methods could be put on an interface, we provide a default implementation, and SteamConfiguration would only have one HTTP-related method/property to get/replace the HTTP configuration object.

[codecov-io]

Codecov Report

Merging #605 into master will increase coverage by 0.49%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
+ Coverage   23.21%   23.71%   +0.49%     
==========================================
  Files          85       85              
  Lines        8672     8717      +45     
  Branches      719      721       +2     
==========================================
+ Hits         2013     2067      +54     
+ Misses       6528     6517      -11     
- Partials      131      133       +2

Impacted Files	Coverage Δ
SteamKit2/SteamKit2/Steam/CDNClient.cs	0% <0%> (ø)	⬆️
...am/SteamClient/Configuration/SteamConfiguration.cs	88% <100%> (+0.5%)	⬆️
...mClient/Configuration/SteamConfigurationBuilder.cs	100% <100%> (ø)	⬆️
...Steam/WebAPI/SteamConfigurationWebAPIExtensions.cs	50% <87.5%> (+25%)	⬆️
SteamKit2/SteamKit2/Steam/WebAPI/WebAPI.cs	41.34% <88.23%> (+0.95%)	⬆️
SteamKit2/SteamKit2/Types/SteamID.cs	79.42% <0%> (-0.37%)	⬇️
SteamKit2/SteamKit2/Util/StreamHelpers.cs	75.58% <0%> (+28.91%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4360d6f...e2d6f88. Read the comment docs.

[xPaw]

Some things that I feel like should be considered, both WebAPI and CDNClient expose timeouts differently, and I think it could be handled with this.

SteamKit/SteamKit2/SteamKit2/Steam/WebAPI/WebAPI.cs

Lines 179 to 182 in 4360d6f

	public TimeSpan Timeout
	{
	get => httpClient.Timeout;
	set => httpClient.Timeout = value;

SteamKit/SteamKit2/SteamKit2/Steam/CDNClient.cs

Line 213 in aa3e7f1

public static TimeSpan RequestTimeout = TimeSpan.FromSeconds( 10 );

And why does CDNClient use cancellation token instead of httpclient's timeout properties?

[yaakov-h]

I think CDNClient uses a cancellation token because it's a direct translation of the old code which used IAsyncResult.AsyncWaitHandle to perform the cancel-on-timeout logic.

I'd be happy to make CDNClient use HttpClient.Timeout instead, but as a separate refactor.

[JustArchi]

Won't this cause issues if consumer is using HttpClient for his own requests with custom base address (or lack of it)?

[yaakov-h]

The consumer shouldn’t be doing that. They can share an underlying HttpMessageHandler, but shouldn’t be sharing the HttpClient itself.

[JustArchi]

It's probably good idea to mention it then, I'd expect from SK2 to use my HttpClient as it is, without modifying its properties such as the base address.

[xPaw]

I would agree here, It should be easy enough to just pass base address into urlBuilder directly instead of using HttpClient.BaseAddress.

[yaakov-h]

The xmldoc calls this out by differentiating between “returns a new or existing XXXXX” and “creates a new XXXXX”.

Sharing an http client outside of our own library would drastically limit how much of its functionality we can actually use, both now and into the future.

[JustArchi]

This is a fair point, if you can see how it'd bite us in the future then I see no need to do any of that. In fact, I'd even vouch for complete removal of custom HttpClient and leave handler only, but sadly there are HttpClient settings that the consumer might want to supply, mainly default request headers for identification purposes.

Maybe it could be a good idea to allow consumer to supply only its own HttpClientHandler and default headers, leaving creating HttpClient to SK2. I don't see any other reason for one to force usage of its own HttpClient, and because SK2 is expected to modify its properties anyway, doing it this way would avoid a lot of confusion (even if it's stated in the doc).

[yaakov-h]

If the consumer wants to re-use HttpMessageHandler instances, then the HttpClient has to be created with dispose: false so that the handler is re-usable and not killed early.

We could make a bool ShouldDisposeOfHandler(HttpMessageHandler handler) method, but that seems oddly specific.

[JustArchi]

It feels fine for me to supply message handler, default headers and value specifying that my handler is supposed to be reused in my own routines further, so it shouldn't be disposed.

Considering that there is really not much more to add towards that (I mean, you've defined everything regarding how to create and use HttpClient by now), I feel that it's a good tradeoff for having a guarantee that HttpClient created by SK2 has expected state and no issues as long as user can guarantee that his message handler is proper.

[JustArchi]

Still, this is more like a design choice over personal preferences rather than some crucial decision, so I leave that up to you, the PR is fine as it is, I just don't see a reason why consumer must create HttpClient and ensure it meets all SK2 requirements and can be modified, if SK2 can pretty much do the same with just 1 more argument (headers + disposing vs httpclient alone).

[yaakov-h]

If the consumer:

• Doesn't supply their own message handler factory
• Supplies a new message handler each time

Then they don't need to create a HttpClient. If they want more advanced control, then I'd rather give them more advanced control rather than re-invent the wheel.

For example, they could use HttpClientFactory with few (if any) modifications.

[yaakov-h]

I'm starting to question whether or not we need Func<HttpMessageHandler>... could we just have Func<HttpClient> as the only overridable function?

[JustArchi]

Sounds good to me and this is what I suggested back then. If you're expecting HttpClient from the consumer then I see no reason why you need handler as well. I mean sure, I know that consumer might provide that one if he has no reason to do anything with HttpClient itself, but then there is no reason why he can't just provide HttpClient right away.

I thought about hypothetical situation in which SK2 would want to do something with handler before creating HttpClient with it, but then you'd need to do breaking change and remove consumer providing HttpClient, as to the best of my knowledge you can't modify handler from HttpClient, and it'd render consumer providing it useless.

So yes, if you can't find any edge case, I think it's a good idea.

[yaakov-h]

Hmmm.

I did some more thinking and, in theory, we could possibly maybe some day want to insert our own DelegatingHandler into the chain.

However, what we could do then, is:

• Construct our own HttpClient with our own HttpMessageHandler
• Have our own HttpMessageHandler do whatever it needs to do, then delegate the actual sending and recieving of the HTTP message to the consumer-provided HttpClient

It's two more object allocations, but if that be comes a problem then we can optimize later down the road some day. No point whatsoever doing that prematurely.

So yeah, it looks like we only need the one of these extensibility points.

I'll refactor.

[yaakov-h]

Any last thoughts or comments?

[rossengeorgiev]

Jillbot has spoken, close dis.