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.

[davidfowl]

I'd like this a similar API for the same reasons. Ideally, I'd be able to replace the "socket" with a connection from this API https://github.com/davidfowl/BedrockTransports#client-api.

[JamesNK]

I think this will be useful for the .NET gRPC client. One of our goals in 5.0 is providing gRPC over named pipes and unix sockets. WCF has the ability to use named pipes for IPC, and this feature will let us address a gap in the gRPC vs WCF comparison.

[simonferquel]

We actually do exactly that at docker: in our golang base we have extensive use of grpc over arbitrary streams (docker build with buildkit for example upgrade an http connection to the build endpoint to raw r/w stream and establish a grpc connection on it, dockerd talks to containers using grpc over Unix sockets as well).
In docker desktop for now we have a somewhat limited implementation of http over stream in C#. This proposal is about introducing the correct abstractions on the client and on the server to be able to leverage the complete asp.net core stack and httpclient implementation with a custom listener/dialer.

[scalablecory]

Yea, I'd like it too as it will help us with benchmarking if we can easily remove I/O from the equation.
NuGet also has a use case in dotnet/corefx#41412.

I prefer the Uri signature because it's more obvious what the arguments are than string and int. I'm not seeing a use in passing the HttpRequestMessage especially considering pooled connections. Also, dotnet/corefx#35410 will obviate the need to return a Socket. So, here's what I'd like:

Func<Uri, CancellationToken, ValueTask<Stream>>

Can we reach consensus? @dotnet/ncl any thoughts?

[stephentoub]

So, here's what I'd like
Can we reach consensus?

I suggest enumerating all known use cases and validating that this design is sufficient to support them. Are there cases where it's important to know whether we're targeting a proxy? What would the recommended approach be if various settings from the handler are needed; access the handler itself by closure? Will this design force allocations that wouldn't otherwise be necessary, e.g. to construct the Uri? Are there scenarios where a dev would need to pass in additional info into the handler, such as via headers in the request message? Etc.

[scalablecory]

A customer has indicated via support channels a want for DNS caching. This feature would allow them to solve that.

[karelz]

Triage: We should do it. Next step we need to verify the API proposal above.

[AArnott]

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.

Having just evaluated adopting gRPC in a large application, I can tell you that we aren't likely to adopt this library unless it added support for operating over a .NET Stream (and/or IDuplexPipe).

One of our goals in 5.0 is providing gRPC over named pipes and unix sockets.

Add support for these things specifically instead of general .NET Stream support means it's more of a hassle to test and higher overhead for scenarios where everything is in-proc.

In particular, I'd like the option to skip the 'dialer' altogether. Let me just pass a .NET Stream to two parties and they can slap the gRPC protocol on top of it and talk to each other.

[davidfowl]

In particular, I'd like the option to skip the 'dialer' altogether. Let me just pass a .NET Stream to two parties and they can slap the gRPC protocol on top of it and talk to each other.

This design needs to account for actual HTTP/1 and HTTP/2 scenarios where actual networking sockets are involved. If you already have a connected socket then this callback would just return the stream directly.

[scalablecory]

Add support for these things specifically instead of general .NET Stream support means it's more of a hassle to test and higher overhead for scenarios where everything is in-proc.

The dialer concept is not mutually exclusive with named pipes and unix domain sockets.

In particular, I'd like the option to skip the 'dialer' altogether. Let me just pass a .NET Stream to two parties and they can slap the gRPC protocol on top of it and talk to each other.

The ability to pass in a stream directly to e.g. SendAsync would be challenging architecture-wise and a little heavyweight (including several new APIs) to make it support multiplexing and keepalive, which others are certainly interested in using through this feature. I think your scenario isn't blocked by a connect callback -- there are some easy workarounds.

[scalablecory]

Prototyping this now for benchmarking of #1590

[scalablecory]

Summary of the use cases we've collected so far:

• Non-TCP transports, such as named pipes or unix domain sockets.
• Custom hostname resolution
  • Caching for environments with unreliable DNS.
  • TTL control, either extending or reducing.
  • Round-robin.
  • Prioritizing certain subnets i.e. in network vs out of network.
  • Filtering out IPv4 or IPv6 addresses.
• Binding to specific IPs/adapters for servers on multiple networks.
• Happy Eyeballs style connect().
• Benchmarking and testing without using actual I/O.
• Transparent filters e.g. bandwidth usage monitoring.

[scalablecory]

Something like @davidfowl's Bedrock might be related work.

[jhudsoncedaron]

So I actually have an implementation request on how I think this should work. I don't want ConnectCallback to be originally initialized to null. I want it to be initialized to the internal default handler. That way, I can write a handler of the form if (server == "myserver") return MyCustomImplemention(); return ImplementationThatWasThereBeforeIRegisteredMineWhichMayBeSystemDefault()

[scalablecory]

@jhudsoncedaron agreed. One goal this aligns with is that it should be very easy to use the default functionality but just add a small shim.

[jtattermusch]

CC @jtattermusch

[gvkries]

I think connection pooling must be taken into account here too. In my case I need to send HTTP messages via a custom transport to several different clients, but they are all using the same host name. If the custom stream gets pooled in this scenario, requests would get sent to the wrong connection.

[tmds]

I think connection pooling must be taken into account here too. In my case I need to send HTTP messages via a custom transport to several different clients, but they are all using the same host name. If the custom stream gets pooled in this scenario, requests would get sent to the wrong connection.

SocketsHttpHandler has some properties already to control the pooling. You can set those on the HttpClient you're using for these requests.

[scalablecory]

I think connection pooling must be taken into account here too. In my case I need to send HTTP messages via a custom transport to several different clients, but they are all using the same host name. If the custom stream gets pooled in this scenario, requests would get sent to the wrong connection.

To do this we'd need to add a pool partition key. I'm not sure if that feature is needed strongly enough to warrant the added API/complexity -- I might suggest handling this via a pseudo-hostname that contains such a key that the custom dialer can can then strip off.

[jhudsoncedaron]

pseudo-hostname

Incidentally that's my plan.

[gvkries]

I might suggest handling this via a pseudo-hostname that contains such a key that the custom dialer can can then strip off.

I might be missing something, but I don't see how this would be possible, if the host name in the request message must be the same but we want to use different transport streams.

[jhudsoncedaron]

@createivbox It looks something like this. https://en.wikipedia.org/wiki/Schwartzian_transform

[gvkries]

It looks something like this. https://en.wikipedia.org/wiki/Schwartzian_transform

Thank you very much, but in my case I need to send the original host value. Replacing the host name for the connection pool only is not possible with the current proposal, because we cannot change it back when it gets transmitted.

A simple additional value in the connection pool key that may be added through HttpRequest properties would solve this, but I understand this is a very rare case.

[vfrz]

I am really looking forward to this feature, is it coming any time soon?

[bbqsrc]

Any progress on this?

[karelz]

Yes, see #1793 (some details about planned API review in #23267 (comment))

[scalablecory]

Closing this -- resolved by #1793.

[scalablecory]

This has been resolved via the API added here in .NET 5: #41949