Make ConnectCallback more user friendly

[ManickaP]

In many slightly advanced scenario, we advise users of HttpClient to use SocketsHttpHandler.ConnectCallback to customize connection creation.
And we suggest copying code from our own sources to get the initial/default implementation for the callback:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

Lines 1585 to 1607 in 9129083

	// Otherwise, create and connect a socket using default settings.
	Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
	try
	{
	if (async)
	{
	await socket.ConnectAsync(endPoint, cancellationToken).ConfigureAwait(false);
	}
	else
	{
	using (cancellationToken.UnsafeRegister(static s => ((Socket)s!).Dispose(), socket))
	{
	socket.Connect(endPoint);
	}
	}
	stream = new NetworkStream(socket, ownsSocket: true);
	}
	catch
	{
	socket.Dispose();
	throw;
	}

This is neither overly long nor complicated code, but it might be dissuading to the users, especially if they just want to do the exactly same thing as we do. Related issues to that: #78070 #77755

To make this easier to the users we could expose something like DefaultConnectCallback, some options for this are:

• https://gist.github.com/MihaZupan/77dc103dff65ca75437f020ad0c106e4
• NetworkStream.CreateFromTcpSocketAsync(DnsEndPoint ep, Action<Socket> configureSocketOptions, CancellationToket ct)
• expose it via [API Proposal]: Simple, modern TCP APIs #63162

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

In many slightly advanced scenario, we advise users of HttpClient to use SocketsHttpHandler.ConnectCallback to customize connection creation.
And we suggest copying code from our own sources to get the initial/default implementation for the callback:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

Lines 1585 to 1607 in 9129083

	// Otherwise, create and connect a socket using default settings.
	Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
	try
	{
	if (async)
	{
	await socket.ConnectAsync(endPoint, cancellationToken).ConfigureAwait(false);
	}
	else
	{
	using (cancellationToken.UnsafeRegister(static s => ((Socket)s!).Dispose(), socket))
	{
	socket.Connect(endPoint);
	}
	}
	stream = new NetworkStream(socket, ownsSocket: true);
	}
	catch
	{
	socket.Dispose();
	throw;
	}

This is neither overly long nor complicated code, but it might be dissuading to the users, especially if they just want to do the exactly same thing as we do. Related issues to that: #78070 #77755

To make this easier to the users we could expose something like DefaultConnectCallback, some options for this are:

• https://gist.github.com/MihaZupan/77dc103dff65ca75437f020ad0c106e4
• NetworkStream.CreateFromTcpSocketAsync(DnsEndPoint ep, Action<Socket> configureSocketOptions, CancellationToket ct)
• expose it via [API Proposal]: Simple, modern TCP APIs #63162

Author:	ManickaP
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[Epic-Santiago]

While the code may not be long, the concern is that when we try to change a setting, we are forced into taking ownership of the socket creation with all its defaults. This is not what the user intends. They just want to change their setting and not own the socket creation. And while the code may be short, .NET will keep evolving and the defaults might change while the copy/pasted code the user has will become outdated and possibly less than optimal (or outright broken) in the future.

I think the measure of success for these changes are: If user U wants to change setting A, can they do that without having to concern themselves with the rest of the socket creation internals? A DefaultConnectCallback should help address that.

[stephentoub]

some options for this are

In the past we've also talked about adding a new member like:

public Func<Socket> CreateSocketCallback { get; set; }

which ConnectCallback would use if set instead of its own socket creation logic, and/or:

public Action<Socket> ConfigureSocketCallback { get; set; }

which the default callback would invoke, passing in the socket it created, for any additional configuration of the socket desired.

[wfurt]

The way it is structured now, there may not even be a socket e.g. the ConnectCallback returns Stream and Http does not use the socket directly AFAIK. And adding more callbacks/actions may not make it easier for users. The need to customize something existing is separate from the need/desire to control the creation but we currently share single mechanism.

If we make progress on #63162 I'm wondering if we can get away with removing all custom connection logic from HTTP. If we could, there would be no doubts about defaults, creation and future deviations.

[stephentoub]

The way it is structured now, there may not even be a socket e.g. the ConnectCallback returns Stream and Http does not use the socket directly AFAIK.

The default implementation creates a Socket; of a callback was provided, that would be used instead. If the developer also provided a ConnectCallback, it'd be up to their implementation to use the socket callback they also provided, but presumably specifying both would be rare. I'm not seeing the problem.

[Epic-Santiago]

The need to customize something existing is separate from the need/desire to control the creation but we currently share single mechanism.

This eloquently articulates what I've been trying to say: from a user point of view, changing a setting and controlling the creation are two very different things and forcing them together is user-unfriendly.

[stephentoub]

I don't see how it's user-unfriendly providing one or the other callbacks I highlighted. It provides a hook that either gives you a Socket to configure or let's you create the one to use.

[Epic-Santiago]

I'm not saying providing callbacks is user unfriendly. I'm saying the current design where I need to overtake the socket creation code just so I can set a timeout is user unfriendly. So the comment was about the current implementation and not the proposed DefaultConnectCallback proposal.

[wfurt]

Since the ConnectCallback creates Stream (or Socket) it feels like form of ConfigureSocketCallback would be sufficient, right? I'm not opposing to it but the benefit seems small to me.

[stephentoub]

Since the ConnectCallback creates Stream (or Socket) it feels like form of ConfigureSocketCallback would be sufficient, right? I'm not opposing to it but the benefit seems small to me.

From my perspective, this whole issue is of small benefit. ConnectCallback already exists as a way to completely customize the establishment of the connection, socket creation, socket connection, all of it, and the canonical boilerplate for doing all of that is:

handler.ConnectCallback = async (ctx, cancellationToken) =>
{
    var socket = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
    try
    {
        await socket.ConnectAsync(ctx.DnsEndPoint, cancellationToken).ConfigureAwait(false);
    }
    catch
    {
        socket.Dispose();
        throw;
    }
    return new NetworkStream(socket, ownsSocket: true);
};

so, 14 lines of code, including the 6 that are just braces. That's the best any feature here could hope to save. I realize others disagree with me on this, but I don't see this as being particularly important.

That said, if the goal here is to make it simpler to configure the socket, then I still like the CreateSocketCallback. It enables something the others don't, the ability to not only configure settings on the socket but also how it's created, which means for example you can choose to limit it to just IPv4 or just IPv6, rather than the dual-mode socket we use today. Our use of it would simply be something like:

Socket socket = _createSocketCallback?.Invoke() ?? new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };

and a developer that wanted to configure whatever options on it could just do:

handler.CreateSocketCallback = ctx =>
{
    var socket = new Socket(... /* do whatever you want here to construct the socket */);
    ...; // do whatever you want here with socket
    return socket;
};

If we didn't care about customizing the constructor and instead just wanted to allow configuration of the already created socket, prior to establishing the connection, we could do:

Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
_configureSocketCallback?.Invoke(socket);

and a developer could do:

handler.ConfigureSocketCallback = socket =>
{
    ...; // do whatever you want here with socket
};

If we don't care about being able to customize the socket's construction or customize the socket prior to connecting, then the DefaultConnectCallback approach could be used.

I'll leave my thoughts at that. I don't personally think saving 8 lines of copy/pastable code is worth more time. But we can pursue whatever approach folks think is best.

[wfurt]

to add more complexity, we also have #64449. We should think how to approach HTTP/3 (and 4, 5, ...)
There may not be enough similarities but it will probably also increase number of callbacks and choices somebody needs to make.

[antonfirsov]

The original end-to-end story here is that something that was as simple as ServicePointManager.SetTcpKeepAlive(true, 100, 200); in .NET framework is now much more complicated - see #77755. For me the essence of this issue is that we should try to make this kind of stuff more accessible. If we want to introduce yet another callback that works with Socket, we also need to make sure to expose simpler API-s for tcp keepalive and other common options on Socket. But I like the #64449 approach better.

[stephentoub]

we also need to make sure to expose simpler API-s for tcp keepalive and other common options on Socket

I don't believe that's a given.

[ManickaP]

Triage: we should decide what to do about this in 8.0 and if we decide to do something, it should be fairly easy to do in this release.

[Epic-Santiago]

How about adding another method to HttpClientBuilderExtensions? The usage would be like this:

public static IHttpClientFactory CreateHttpClientFactory()
    => new ServiceCollection()
        .AddHttpClient("Test")
        .SetTcpKeepAlive(true, 100, 200)

This adds back the ease of use without altering the API. It's an encapsulation of the currently copy/pasted code without changing HttpClient/HttpClientBuilder itself. The main advantage is that the currently copy/pasted code would be moved into the framework thus relieving the users from having to keep that code up-to-date. And it also addresses the usability without adding more callbacks.

[CarnaViire]

This will only apply to HttpClientFactory @Epic-Santiago, and not everyone need/can use HttpClientFactory (e.g. consider apps without DI) -- we need to take into account users of plain HttpClient as well.
Also, TCP Keep-Alive is just one of potential things to configure in a socket.

(Returning to HttpClientFactory proposal, in case of a user-provided custom primary handler, there's no way to honor this setting, so it's usability will be limited to SocketsHttpHandler, and only as a default primary handler, not a user-provided one. Which, in my opinion, implies that this setting is better to be configured on SocketsHttpHandler itself)

[GSPP]

Taken from https://gist.github.com/MihaZupan/77dc103dff65ca75437f020ad0c106e4#file-connectcallback-cs-L20:

async ValueTask<Stream> ConnectAsync(SocketsHttpConnectionContext context, CancellationToken cancellationToken)
{
    var networkStream = await context.DefaultConnectAsync(cancellationToken);
    networkStream.Socket.SetFoo(bar); //<=== needs catch
    return networkStream;
}

This, too, must be wrapped in a catch ensuring disposal in case of error. If this is not done, very nasty leaks can result. Unfortunately, this means that the new proposed API is not quite as terse and easy to use as one would hope for.

Stephen's socket customization callback would eliminate that cleanly. That could then be used in a truly simple way:

    socket => socket.SetFoo(bar)

And that's a solution that is Stack Overflow ready in it's simplicity 😜

[ManickaP]

Triage: this doesn't functionally blocks anyone, it's nice to have, moving to future for now.