Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make HTTP components configurable #605

Merged
merged 8 commits into from
Nov 4, 2018
Merged

Make HTTP components configurable #605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d5751b3
Add HTTP factory options to SteamConfiguration
yaakov-h Oct 28, 2018
123d0e2
Use HttpClient factory methods where possible
yaakov-h Oct 28, 2018
3b8d105
Expose new methods in configuration builder
yaakov-h Oct 28, 2018
24f963b
Add tests for new SteamConfiguration behaviour
yaakov-h Oct 28, 2018
d494c82
Remove stray blank line
yaakov-h Oct 28, 2018
35ec168
This should fix the test on Linux...
yaakov-h Oct 28, 2018
65279cc
Remove HttpMessageHandler extensibility point.
yaakov-h Nov 2, 2018
e2d6f88
Missed a spot
yaakov-h Nov 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion SteamKit2/SteamKit2/Steam/CDNClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ public CDNClient( SteamClient steamClient, byte[] appTicket = null )
}

this.steamClient = steamClient;
this.httpClient = new HttpClient();
this.httpClient = steamClient.Configuration.HttpClientFactory();

this.depotIds = new ConcurrentDictionary<uint, bool>();
this.appTicket = appTicket;
Expand Down
8 changes: 8 additions & 0 deletions SteamKit2/SteamKit2/Steam/SteamClient/Configuration/ISteamConfigurationBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@


using System;
using System.Net.Http;
using SteamKit2.Discovery;

namespace SteamKit2
Expand Down Expand Up @@ -44,6 +45,13 @@ public interface ISteamConfigurationBuilder
/// <returns>A builder with modified configuration.</returns>
ISteamConfigurationBuilder WithDirectoryFetch(bool allowDirectoryFetch);

/// <summary>
/// Configures this <see cref="SteamConfiguration" /> with custom HTTP behaviour.
/// </summary>
/// <param name="factoryFunction">A function to create and configure a new HttpClient.</param>
/// <returns>A builder with modified configuration.</returns>
ISteamConfigurationBuilder WithHttpClientFactory(HttpClientFactory factoryFunction);

/// <summary>
/// Configures how this <see cref="SteamConfiguration" /> will be used to connect to Steam.
/// </summary>
Expand Down
14 changes: 14 additions & 0 deletions SteamKit2/SteamKit2/Steam/SteamClient/Configuration/SteamConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,18 @@


using System;
using System.Net.Http;
using SteamKit2.Discovery;

namespace SteamKit2
{
/// <summary>
/// Factory function to create a user-configured HttpClient.
/// The HttpClient will be disposed of after use.
/// </summary>
/// <returns>A new <see cref="HttpClient"/> to be used to send HTTP requests.</returns>
public delegate HttpClient HttpClientFactory();

/// <summary>
/// Configuration object to use.
/// This object should not be mutated after it is passed to one or more <see cref="SteamClient"/> objects.
Expand Down Expand Up @@ -66,6 +74,12 @@ internal static SteamConfiguration CreateDefault()
/// when calling <c>SteamFriends.RequestFriendInfo</c> without specifying flags.
/// </summary>
public EClientPersonaStateFlag DefaultPersonaStateFlags => state.DefaultPersonaStateFlags;

/// <summary>
/// Factory function to create a user-configured HttpClient.
/// </summary>
public HttpClientFactory HttpClientFactory => state.HttpClientFactory;

/// <summary>
/// The supported protocol types to use when attempting to connect to Steam.
/// </summary>
Expand Down
11 changes: 11 additions & 0 deletions SteamKit2/SteamKit2/Steam/SteamClient/Configuration/SteamConfigurationBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@


using System;
using System.Net.Http;
using SteamKit2.Discovery;

namespace SteamKit2
Expand All @@ -29,6 +30,8 @@ public static SteamConfigurationState CreateDefaultState()
EClientPersonaStateFlag.SourceID | EClientPersonaStateFlag.GameExtraInfo |
EClientPersonaStateFlag.LastSeen,

HttpClientFactory = DefaultHttpClientFactory,

ProtocolTypes = ProtocolTypes.Tcp,

ServerListProvider = new NullServerListProvider(),
Expand Down Expand Up @@ -68,6 +71,12 @@ public ISteamConfigurationBuilder WithDirectoryFetch(bool allowDirectoryFetch)
return this;
}

public ISteamConfigurationBuilder WithHttpClientFactory(HttpClientFactory factoryFunction)
{
state.HttpClientFactory = factoryFunction;
return this;
}

public ISteamConfigurationBuilder WithProtocolTypes(ProtocolTypes protocolTypes)
{
state.ProtocolTypes = protocolTypes;
Expand Down Expand Up @@ -97,5 +106,7 @@ public ISteamConfigurationBuilder WithWebAPIKey(string webApiKey)
state.WebAPIKey = webApiKey ?? throw new ArgumentNullException(nameof(webApiKey));
return this;
}

static HttpClient DefaultHttpClientFactory() => new HttpClient();
}
}
1 change: 1 addition & 0 deletions SteamKit2/SteamKit2/Steam/SteamClient/Configuration/SteamConfigurationState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ struct SteamConfigurationState
public uint CellID;
public TimeSpan ConnectionTimeout;
public EClientPersonaStateFlag DefaultPersonaStateFlags;
public HttpClientFactory HttpClientFactory;
public ProtocolTypes ProtocolTypes;
public IServerListProvider ServerListProvider;
public EUniverse Universe;
Expand Down
15 changes: 13 additions & 2 deletions SteamKit2/SteamKit2/Steam/WebAPI/SteamConfigurationWebAPIExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Net.Http;

namespace SteamKit2
{
Expand All @@ -20,7 +21,7 @@ public static WebAPI.Interface GetWebAPIInterface(this SteamConfiguration config
throw new ArgumentNullException(nameof(config));
}

return new WebAPI.Interface(config.WebAPIBaseAddress, iface, config.WebAPIKey);
return new WebAPI.Interface(config.GetHttpClientForWebAPI(), iface, config.WebAPIKey);
}

/// <summary>
Expand All @@ -36,7 +37,17 @@ public static WebAPI.AsyncInterface GetAsyncWebAPIInterface(this SteamConfigurat
throw new ArgumentNullException(nameof(config));
}

return new WebAPI.AsyncInterface(config.WebAPIBaseAddress, iface, config.WebAPIKey);
return new WebAPI.AsyncInterface(config.GetHttpClientForWebAPI(), iface, config.WebAPIKey);
}

internal static HttpClient GetHttpClientForWebAPI(this SteamConfiguration config)
{
var client = config.HttpClientFactory();

client.BaseAddress = config.WebAPIBaseAddress;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@JustArchi JustArchi Oct 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@JustArchi JustArchi Oct 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@JustArchi JustArchi Oct 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

@yaakov-h yaakov-h Oct 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

client.Timeout = WebAPI.DefaultTimeout;

return client;
}
}
}
34 changes: 21 additions & 13 deletions SteamKit2/SteamKit2/Steam/WebAPI/WebAPI.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ public sealed class WebAPI
/// </summary>
public static Uri DefaultBaseAddress { get; } = new Uri("https://api.steampowered.com/", UriKind.Absolute);

internal static TimeSpan DefaultTimeout { get; } = TimeSpan.FromSeconds(100);

/// <summary>
/// Represents a single interface that exists within the Web API.
/// This is a dynamic object that allows function calls to interfaces with minimal code.
Expand All @@ -48,9 +50,9 @@ public TimeSpan Timeout
}


internal Interface( Uri baseAddress, string iface, string apiKey )
internal Interface( HttpClient httpClient, string iface, string apiKey )
{
asyncInterface = new AsyncInterface( baseAddress, iface, apiKey );
asyncInterface = new AsyncInterface( httpClient, iface, apiKey );
}


Expand Down Expand Up @@ -187,14 +189,9 @@ public TimeSpan Timeout
RegexOptions.Compiled | RegexOptions.IgnoreCase
);

internal AsyncInterface( Uri baseAddress, string iface, string apiKey )
internal AsyncInterface( HttpClient httpClient, string iface, string apiKey )
{
httpClient = new HttpClient
{
BaseAddress = baseAddress,
Timeout = TimeSpan.FromSeconds(100)
};

this.httpClient = httpClient;
this.iface = iface;
this.apiKey = apiKey;
}
Expand Down Expand Up @@ -445,7 +442,7 @@ public static Interface GetInterface( Uri baseAddress, string iface, string apiK
throw new ArgumentNullException( nameof(iface) );
}

return new Interface( baseAddress, iface, apiKey );
return new Interface( CreateDefaultHttpClient( baseAddress ), iface, apiKey );
}

/// <summary>
Expand All @@ -461,7 +458,7 @@ public static Interface GetInterface( string iface, string apiKey = "" )
throw new ArgumentNullException( nameof(iface) );
}

return new Interface( DefaultBaseAddress, iface, apiKey );
return new Interface( CreateDefaultHttpClient( DefaultBaseAddress ), iface, apiKey );
}

/// <summary>
Expand All @@ -477,7 +474,7 @@ public static AsyncInterface GetAsyncInterface( string iface, string apiKey = ""
throw new ArgumentNullException( nameof(iface) );
}

return new AsyncInterface( DefaultBaseAddress, iface, apiKey );
return new AsyncInterface( CreateDefaultHttpClient( DefaultBaseAddress ), iface, apiKey );
}

/// <summary>
Expand All @@ -499,7 +496,18 @@ public static AsyncInterface GetAsyncInterface( Uri baseAddress, string iface, s
throw new ArgumentNullException( nameof(iface) );
}

return new AsyncInterface( baseAddress, iface, apiKey );
return new AsyncInterface( CreateDefaultHttpClient( baseAddress ), iface, apiKey );
}

static HttpClient CreateDefaultHttpClient( Uri baseAddress )
{
var client = new HttpClient
{
BaseAddress = baseAddress,
Timeout = DefaultTimeout
};

return client;
}
}

Expand Down
24 changes: 24 additions & 0 deletions SteamKit2/Tests/SteamConfigurationFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;
using SteamKit2;
using SteamKit2.Discovery;
Expand Down Expand Up @@ -44,6 +48,16 @@ public void DefaultPersonaStateFlags()
Assert.Equal(expected, configuration.DefaultPersonaStateFlags);
}

[Fact]
public void DefaultHttpClientFactory()
{
using (var client = configuration.HttpClientFactory())
{
Assert.NotNull(client);
Assert.IsType<HttpClient>(client);
}
}

[Fact]
public void ServerListProviderIsNothingFancy()
{
Expand Down Expand Up @@ -90,6 +104,7 @@ public SteamConfigurationConfiguredObjectFacts()
.WithCellID(123)
.WithConnectionTimeout(TimeSpan.FromMinutes(1))
.WithDefaultPersonaStateFlags(EClientPersonaStateFlag.SourceID)
.WithHttpClientFactory(() => { var c = new HttpClient(); c.DefaultRequestHeaders.Add("X-SteamKit-Tests", "true"); return c; })
.WithProtocolTypes(ProtocolTypes.WebSocket | ProtocolTypes.Udp)
.WithServerListProvider(new CustomServerListProvider())
.WithUniverse(EUniverse.Internal)
Expand Down Expand Up @@ -117,6 +132,15 @@ public void ConnectionTimeoutIsConfigured()
Assert.Equal(TimeSpan.FromMinutes(1), configuration.ConnectionTimeout);
}

[Fact]
public void HttpClientFactoryIsConfigured()
{
using (var client = configuration.HttpClientFactory())
{
Assert.Equal("true", client.DefaultRequestHeaders.GetValues("X-SteamKit-Tests").FirstOrDefault());
}
}

[Fact]
public void PersonaStateFlagsIsConfigured()
{
Expand Down