Add ConnectCallback on SocketsHttpHandler (aka custom Dialers)

[simonferquel]

Summary

The current SocketHttpHandler implementation cleanly separates the HTTP grammar to the actual TCP connection handling. However, while Kestrel on the server side allows to expose a service over non-TCP protocols such as Unix Sockets (and probably named pipes as well on Windows), the HttpClient does not provide such functionality.
As the SocketHttpHandler main logic only depends on Socket and Stream (and actually has a fallback mechanism to be able to work with no Socket, and just a Stream), this proposal is about adding custom Dialers to the SocketHttpHandler, allowing a user to use whatever transport he wants to connect to the server.
Also, there is no way to reuse all the Http grammar goodness implemented in System.Net.Http in a custom MessageHandler in a foreign assembly, because all this is internal. One potential alternative to this proposal would then be to refactor all this internal code in pieces consumable from a user.

Proposed API

This Proposal adds two properties to SocketHttpHandler:

public sealed partial class SocketsHttpHandler : System.Net.Http.HttpMessageHandler
{
// < ... >
    public Func<HttpRequestMessage, Threading.CancellationToken, Threading.Tasks.ValueTask<IO.Stream>> ConnectCallback { get { throw null; } set { } }
}

Usage

var handler = new SocketsHttpHandler{
   ConnectCallback = async (HttpRequestMessage message, CancellationToken cancellationToken)=>{
      // custom logic here
      // ....
      //
      return stream
   }
};
var client = new HttpClient(handler);
var response = await client.GetAsync(uri);

Open Questions

• We could only use a single Dialer property, but it would not be obvious to the user that it is actually ok to not return a Socket. -> Only one property now. If required, we can introduce an IWithSocket interface that the returned stream might implement to provide raw socket access to the SocketsHttpHandler
• C# 8 constructs such as public Func<string, int, Threading.CancellationToken, Threading.Tasks.ValueTask<(Sockets.Socket?, IO.Stream)>> could help, but I am not sure how this kind of constructs is exposed to other users. Single stream return value (optionally implementing IWithSocket) fixes the question
• Another suggestion, is to introduce a delegate type for this Dialer callback with explicit documentation about the optional nature of the Socket return value. Ruled out by single return value + HttpRequestMessage in the callback parameter
• Yet another one would be to have a single Dialer callback property, and add an extension method WithStreamDialer on SocketsHttpHandler that would wrap a "Stream only" callback. No extension method should be introduced

Reference

WIP implementation: dotnet/corefx@master...simonferquel:custom-dialers
Prototype of previous related work done for Docker Desktop: https://github.com/simonferquel/HttpOverStream/tree/master/src/HttpOverStream (illustrate the difficulty of implementing a custom MessageHandler without access to internal code from System.Net.Http)

[simonferquel]

ping @karelz, @stephentoub

[bartonjs]

Our current API guidance says to not use tuples in public API. (Either make a named struct, or make a custom delegate type to use an out parameter)

I didn't personally find the int parameter obvious as port until seeing an example, which makes me wonder if a custom delegate is beneficial anyways.

[simonferquel]

@bartonjs The more I think about it, the more I think introducing a custom delegate makes sense for parameter naming. Introducing a struct for the result of the Dialer, with a constructor taking only a Stream would certainly make things more obvious (about optionality of the Socket return value).

Wdyt ?

[stephentoub]

A few thoughts:

• One property for this is much better than two, both for usability and for the implementation (it has to check both, it's otherwise ambiguous which should be used if both are set, etc.)
• "Dialer" isn't a great name here. This should instead be something like "ConnectCallback" or "OpenConnectionStreamCallback" or something along those lines, to map to the other callbacks already in the APIs.
• The Socket that's returned from ConnectHelper today is there as an optimization, to make it easier for SocketsHttpHandler to poll a pooled connection to see whether it's still viable. However, there's a fallback path that works for arbitrary streams as well. We might just want to drop it from this API, and SocketsHttpHandler can fish out the Socket when it's used with a Stream type it knows about, e.g. either adding a public Socket-returning property to NetworkStream, or more likely just having the default connect callback return a DerivedNetworkStream that exposes the socket to SocketsHttpHandler; it can then do a type check/cast to get the socket in cases it knows about. That avoids the complication in the API, avoids the need for a tuple, and generally just keeps the API simple: give us back a stream.
• We should think about whether there are additional pieces of information that should be passed in. Would it be better / worse for the Uri or HttpRequestMessage to be passed instead of host/port (even though the connection is likely to be reused for other messages/uris on the same port)? Should the SocketsHttpHandler itself be passed in as an argument, or would it be sufficient for the callback to capture any information it needs from the handler via a closure? This would also further obviate the need for a custom delegate, as it's very clear what things are, e.g. Func<Uri, CancellationToken, Stream>. That said, even if we stick with host/port (which might be the right answer), I'd still prefer Func<string, int, CancellationToken, Stream> over a custom delegate.

Finally, I expect very few developers will use this property. It's a reasonable extensibility point to add, but I would prioritize keeping the API as clutter-free as possible (e.g. not adding a lot of helper types) rather than trying to make the API super easy to use.

[simonferquel]

@stephentoub thanks, updated the ProposedAPI with your suggestions.

[geoffkizer]

Related discussions:
#25401
#23267

[geoffkizer]

FYI, "dialer" is a term used in Go. I don't love it either, just providing context.

[stephentoub]

@simonferquel, regarding https://twitter.com/sferquel/status/1097893536412979200, yes, this would only be for .NET Core.

[simonferquel]

@stephentoub hmm, that is bad news for Docker Desktop (which runs on the full .Net Framework, and won't easily move to .net core).
I guess I'll have to try and build the SocketsHttpHandler separately on top of .net 4.6 then (a quick pass trough the code feels like it is doable. if I could automate this, it would be great)

[simonferquel]

Well, not so easy at all after all, devil is in the detail.

[simonferquel]

ping @stephentoub @karelz, I see it is in milestone future, do you have a status for it (like should I pursue efforts on this)?

[stephentoub]

@simonferquel, we're still interested, this just hasn't been at the top of our list. I doubt it'll make it for 3.0, but I'll defer to @karelz on that.

[simonferquel]

Thanks for the heads-up, no problem for skipping 3.0. docker desktop is far from ready to be ported to .net core anyway :)

[karelz]

The issues linked above by Geoff are on my top list for post-3.0. I have been thinking about them just recently.
Some kind of ConnectionCallback is what I have been thinking about. Not sure if we want to go as deep as providing any Stream (maybe more lower-level APIs would be more appropriate), but we can definitely discuss it when we are ready to invest in the space.

[scalablecory]

If we were to pass in a HttpRequestMessage, I would pass it in addition to the host/port:

Func<string, int, HttpRequestMessage, Threading.CancellationToken, Threading.Tasks.ValueTask<IO.Stream>>

This is because there is some complex proxy logic that users would need to otherwise worry about, and we'd need to expose additional APIs to enable them to worry about it.