Update documentation detailing how to use octokit.net in a thread-safe way

[Cyl18]

I can't find it anywhere

[un1r8okq]

I’ve been wondering this too. I have an ASP.NET Core application using a GitHub client through the DI container. Should it be a singleton to prevent socket exhaustion or transient to prevent concurrency bugs?

[asklar]

it looks like at the end up the day the octokit api will call IHttpClient, which is thread safe (it can handle calls from multiple threads)

[un1r8okq]

@asklar not quite, look a layer deeper. The IHttpClient in Octokit is not compile time compatible with System.Net.Http.HttpClient.

The GitHubClient constructor uses a Connection which does have a constructor accepting an IHttpClient, however, that is Octokit's own interface that it has defined. This is not the HttpClient you're familiar with (which has some thread-safe methods). Instead, the only class inheriting from IHttpClient is their HttpClientAdapter. That wraps the System.Net.Http.HttpClient that you're familar with. Importantly, its constructor allows you to specify a way of getting the HttpMessageHandler that will be passed to the HttpClient that the HttpClientAdapter creates. This is where you would pass in a SocketsHttpHandler to share a connection pool between instances of GitHubClient. So your code could look like this:

using Octokit;
using Octokit.Internal;

services.AddSingleton<IConnection>(_ =>
{
    var socketsHttpHandler = new System.Net.Http.SocketsHttpHandler();
    return new Connection(
        new ProductHeaderValue("MyProduct"),
        new Uri("https://api.github.com"),
        new InMemoryCredentialStore(Credentials.Anonymous),
        new HttpClientAdapter(() => socketsHttpHandler),
        new SimpleJsonSerializer());
});

Then you have your constructors accept an IGitHubClient where needed, and the dependency injection container will resolve to the singleton instance of IConnection, reusing your SocketsHttpHandler and thus reusing your connection pool.

[nickfloyd]

@un1r8okq Thank you for the response here. At a minimum, we need to get your response in the usage docs. Still, we should reevaluate this implementation given that the original use case for it (one of which was windows mobile) is out of date - there could still be value to having this, but we should still take a look. Thanks again for the info that you provided here ❤️.

[nickfloyd]

• [ ] Update docs to reflect the info above