Remove SocketsHttpConnectionFactory

[scalablecory]

SocketsHttpConnectionFactory was proposed as part of #1793.

Its original intent was to provide a number of callbacks for users to customize things without understanding all of the ConnectionFactory stuff. However, API review removed all of these callbacks, making this class largely redundant with SocketsConnectionFactory.

The only remaining features it has are:

• it pulls the HttpRequestMessage out of the property bag for the user.
• it turns Nagle off.

We should consider reducing our API surface by doing the following:

• Remove SocketsHttpConnectionFactory.
• Update SocketsConnectionFactory to default its sockets with Nagle off.
  • This may cause some confusion to people expecting the default Socket behavior. But, it may be okay to be opinionated here, and most experts end up turning it off anyway.
• Update the DnsEndPointWithProperties type to expose an HttpRequestMessage property.

Old usage example:

class MyCustomFactory : SocketsHttpConnectionFactory
{
    public override ValueTask<Connection> EstablishConnectionAsync(HttpRequestMessage message, EndPoint endPoint, IConnectionProperties properties, CancellationToken cancellationToken)
    {
       // ...
    }
}

New usage example:

class MyCustomFactory : SocketsConnectionFactory
{
    public override ValueTask<Connection> ConnectAsync(EndPoint endPoint, IConnectionProperties properties, CancellationToken cancellationToken)
    {
        if(!properties.TryGet(out HttpRequestMessage message)) throw new Exception("Expected HttpRequestMessage property");
        // ...
    }
}

CC @geoffkizer @karelz this came about from a discussion with @stephentoub. I feel we can treat this as low priority, but it is a low-effort way to improve API surface.

[ghost]

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

[geoffkizer]

This seems like a good thing.

But I don't understand this part:

Update the DnsEndPointWithProperties type to expose an HttpRequestMessage property.

[scalablecory]

This seems like a good thing.

But I don't understand this part:

Update the DnsEndPointWithProperties type to expose an HttpRequestMessage property.

We currently have this code:

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

Lines 19 to 29 in 7acaf16

	bool IConnectionProperties.TryGet(Type propertyKey, [NotNullWhen(true)] out object? property)
	{
	if (propertyKey == typeof(DnsEndPointWithProperties))
	{
	property = this;
	return true;
	}
	property = null;
	return false;
	}

It exposes a property using an internal type that SocketsHttpConnectionFactory understands. We just need to update this to expose the HttpRequestMessage as a property instead of this internal type.

[geoffkizer]

Yeah, that makes sense, thanks. That seems like it ends up in a better place anyway.

[geoffkizer]

@antonfirsov I believe you're already working on this