[RFC] HttpClientAdapter and Octokit - Future Plans

[shiftkey]

The Setup

You've probably never looked at HttpClientAdapter before. That's because it's buried deep inside Octokit. But that's not been a deliberate thing - it's just the central place for actually doing the leg work of creating, sending and receiving HTTP requests.

If you wanted to create it by hand in Octokit today, this is the code you need:

var connection = new Connection(new ProductHeaderValue("ha-ha-business"), new HttpClientAdapter());
var client = new GitHubClient(connection);

But anyway, let me start with a few notes:

• Repository Redirects is currently being tested in the GitHub API - I'd love to support this but it's currently not possible without reworking HttpClientAdapter. 🚢ed
• Credentials in Octokit.net are very basic (you can only set it once, and it's not tied to a specific domain).
• I have some experiments using Octokit that require swapping out internals, so I'm acutely feeling this pain. And Implementing a IHttpClient leads to lots of code copy #729 was raised a while ago in a similar vein.

The Confrontation

As there's many intertwined concepts in here, I'm gonna try and just start from the beginning:

HttpClient and You

Initially we used HttpClientAdapter to abstract away all the HttpClient-ness. And that's fine. But IHttpClient is just one method (very similar to HttpClientAdapter), it has it's own abstraction for IRequest/IResponse and the conversion to HttpRequestMessage/HttpResponseMessage is a closely guarded secret just kept internal.

If we extract the message building logic out of HttpClientAdapter you're just left with the simple wrapper over HttpClient. And that's okay, maybe we don't need to 🔥 it off right away. But I think that's some low-hanging fruit to do to appease the SRP gods.

Use HttpClientHandler Right

The other thing that HttpClientAdapter handles is the setup of HttpClient. In hindsight, there is a better place for much of this code - a custom implementation of HttpClientHandler! This could be passed in as a ctor parameter, removing yet another responsibility of HttpClientAdapter.

There's one issue I have with this approach, and it's the global Timeout property on HttpClient - there doesn't seem to be an equivalent at the handler level. I dislike leaning on this crutch but it's been useful when uploading releases, so I'm cautious about migrating away from this.

A thought I had was to combine the received cancellation token with a timeout-based cancellation token, like this:

var timeoutCancellation = new CancellationTokenSource(request.Timeout);
var unifiedCancellationToken = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellation.Token);
...
var responseMessage = await http.SendAsync(requestMessage, HttpCompletionOption.ResponseContentRead, unifiedCancellationToken.Token)
    .ConfigureAwait(false);

I'm not even sure this will work, but I'm keen to try it out as it takes away one of my major pain points with HttpClient as-is.

Pluggable HttpClientHandler For All

And now we get into the fun stuff. For a side-project I wanted to implement a caching version of HttpClientHandler to store responses on-disk for a period of time.

This is approximately what the code for that handler would look like:

public class CachingOctokitHttpClientHandler : OctokitHttpClientHandler
{
    readonly IBlobCache _blobCache;

    public CachingOctokitHttpClientHandler(IBlobCache blobCache)
    {
        _blobCache = blobCache;
    }

    protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        return _blobCache.GetOrFetchObject(
                request.RequestUri.AbsoluteUri,
                () => base.SendAsync(request, cancellationToken),
                DateTimeOffset.Now.AddMinutes(5))
            .ToTask(cancellationToken);
    }
}

Note: I bet HttpResponseMessage isn't serializable and this is all just going to end in tears. But again, I can test this out first.

Note: as @paulcbetts asked me about over Twitter, I think HttpClientAdapter could depend on HttpMessageHandler - but the defaults would be built off HttpClientHandler particularly to get GZip/Deflate compression wherever possible.

We currently have a lot of overloads on GitHubClient ctors, and I'd like to review whether we can add deprecated in favour of scenarios like this.

CredentialsCache And You

Lastly, there's how credentials work.

I've included discussions around credentials here because this is another area that HttpClientHandler does well - so perhaps we can deprecate our code for managing credentials and just defer to whatever is in the handler. I'm kinda undecided on this, but our current setup is limited in two significant scenarios:

• redirects which are auto-followed will clear the Authorization header - see MSDN if you don't believe me.
• requests, irrespective of their domain, will have the same credentials set. That's crazy. We can be smarter about that.

The general advice here seems to be to use System.Net.AuthenticationManager from this process because this only seems to exist on .NET 4.5. I've got an alternative way to do this which covers all platforms, but feels primitive.

As an end-user, I really don't want you to care about setting these things up - so how does this feel as a hypothetical API?

var client = new GitHubClient(new ProductHeaderValue("my-cool-app"));
// we can infer the host you want to use based on the current setup of GitHubClient
client.Configuration.Credentials.AddBasic("username", "password");
// but if you want to set it manually, we won't stand in the way 
client.Configuration.Credentials.AddBasic("username", "password", new Uri("https://enterprise.mywork.com"));
// and the same thing can work for tokens
client.Configuration.Credentials.AddToken("my-token-here");
client.Configuration.Credentials.AddToken("my-token-here", new Uri("https://enterprise.mywork.com"));

By making this more configurable, you can use HttpClientAdapter directly for making requests to generic HTTP resources (like images) but without being able to control authentication better, you're at the mercy of hoping resources do not require authorization

A Potential Roadmap

So let's say this is considered a thing worth investing in. How can we break up this big task into multiple, manageable tasks:

Drop the dependency on HttpClient.Timeout

• Prove that you can use a CancellationToken with a timeout instead.
• Swap out the internal behaviour to remove the dependency
• No longer create HttpClient per-request 🔪 🎉 💃

This was completed in #796 and will ship in v0.11

Change HttpClientAdapter to take in a HttpMessageHandler

• extract the default parameters into a custom implementation
• set this as the default ctor dependency
• document more of the internals so others can setup their own usages

Implement redirect handling (with correct authentication handling)

• enhance the default HttpMessageHandler implementation to support processing redirects

When we get to here, we have made the work required to extract the mapping unnecessary (because you don't need to plug in there any more) however the credentials work remains unclear.

cc @haacked when you get a moment to unwind over a nice Manhattan
cc @darrelmiller and @akoeplinger for API and HttpClient feels
cc @forki @distantcam @thedillonb for end-user feedback

[haacked]

This is on my list, but it will take me some time to really think it through. Corner me at BUILD. 😄 🍸

[shiftkey]

Corner me at BUILD. 😄 🍸

Word 🍸

[pavel-b-novikov]

(passing by) Just encountered with support of repository redirects. Forced to checkout sources and "invent crutch" there making IRequest mutable in part of endpoint.
You should write that redirects are not supported on main page.
Actually redirects works but loosing auth information. And in case of bulk repo queries it leads to "rate limit" exception.
Should I pull-request mine lonely crutch in such good code?

[shiftkey]

Should I pull-request mine lonely crutch in such good code?

Let's discuss your workaround in a different thread to this. It's on my roadmap, I'm just trying to figure out the best way to get there right now...

[darrelmiller]

Regarding the redirect issue, I've been using this https://gist.github.com/darrelmiller/a3baeb6976acd9f23243 in one of my client applications to replace the autoredirect done by HttpWebRequest. It could use some love, but it might be an ok starting point.

[darrelmiller]

@shiftkey Take a look at this and let me know what you think darrelmiller@e5d7d6e

I'm having trouble getting the tests to run at the moment, so it isn't tested. Just consider it an idea.

I optionally allow passing in a HttpMessageHandler into HttpClientAdapter. I store a HttpClient instance as a field and reuse it.

In order to allow auto redirects to be turned on and off, I use the HttpRequestMessage.Properties collection to relay the setting to the new redirect message handler.

[shiftkey]

Update: aiming to land repository redirects over in #808 as a way to figure out the HttpMessageHandler API

[shiftkey]

With #808 almost ready to go out the door, I found myself with a quiet moment to reflect on how to improve the initialization of GitHubClient - there's a number of overloads available which take different parameters, and it's not quite easy to navigate.

I'd like to simplify this down to a number of options which are set by the user (this is just a demo):

var options = new ClientOptions
{
    AppName = "my-cool-app",
    Credentials = new Credentials("my-token"),
    Server = "https://enterprise.my-work.com",
    UseDefaultProxy = true
};

And as these are all HTTP-specific, they could be transformed into a HttpMessageHandler

var handler = OctokitMessageHandlerFactory.Create(options);
var client = new GitHubClient(handler);

This could also be the extensibility hook for doing more flexibile things at the HTTP level:

var handler = OctokitMessageHandlerFactory.Create(options);
var cachingHandler = new CachingMessageHandler { InnerHandler = handler };
var client = new GitHubClient(cachingHandler);

I need to think a bunch about how to transition things carefully, but the wheels are already turning...

[darrelmiller]

I like the fact that your proposed GitHubClient extensibility model mirrors the standard HttpClient model. It hides all the IHttpClient, IConnection, HttpClientAdapter stuff from someone who just wants to plug in an extra piece of middleware.

[shiftkey]

@darrelmiller

from someone who just wants to plug in an extra piece of middleware.

That's something I've had in mind for a while (@haacked jump in here if you disagree with this direction) and ultimately we're re-implementing a bunch of the middleware currently - and I'd just love to get out of the way of those who want to dive in.

The ClientOptions could be a constructor interface on top of this, for consumers who don't want to go deep. There's some assumptions we've made (authentication, user-agent, etc) that these brave souls should be aware of, but I think a good goal here would be "here's my handler, plug this into Octokit" so that everything just works...

[haacked]

(@haacked jump in here if you disagree with this direction)

May I jump in here if I don't disagree? ✨

[haacked]

I like the ClientOptions route. It reminds me of the the ProcessStartInfo approach. In fact, maybe we could call it ClientInfo or something like that.

I do think we should retain the simpler .ctors with the most common options.

[shiftkey]

@haacked

I do think we should retain the simpler .ctors with the most common options.

You want to port the existing ctors on GitHubClient over to this ClientInfo class?

[shiftkey]

Seeing how we're all roughly on the same page here, I'm going to open up a new issue to track the roadmap and break down the work better.

[haacked]

You want to port the existing ctors on GitHubClient over to this ClientInfo class?

Not necessarily. I just mean I want to retain some of the existing ctors along with new ones that accept ClientInfo

[shiftkey]

@haacked okay, I'll keep that in mind