Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SocketsHttpHandler feature flag #39489

Closed
JamesNK opened this issue Jul 17, 2020 · 9 comments · Fixed by #40271 or #40308
Closed

SocketsHttpHandler feature flag #39489

JamesNK opened this issue Jul 17, 2020 · 9 comments · Fixed by #40271 or #40308
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Jul 17, 2020

Background and Motivation

If you are creating a cross-platform client today you are probably targeting netstandard2.x and using HttpClientHandler. This handler works well everywhere: .NET Core, Blazor WebAssembly, mobile, etc.

Now if you want to use the more advanced configuration on SocketsHttpHandler you can multi-target netcoreapp3.1 and use #ifdef netcoreapp3.1 to create and configure SocketsHttpHandler, while still using HttpClientHandler in netstandard2.x.

With .NET 5, net5.0 is becoming the general purpose cross-platform target. This means that you can no longer use #ifdef to conditionally use SocketsHttpHandler on only platforms that support it.

A real world example of this problem is the gRPC client. With net5.0 I want the gRPC client to create SocketsHttpHandler internally. SocketsHttpHandler will be configured to allow h2c by default, and create new connections when under heavy load. Unfortunately it will now break when used in Blazor WebAssembly because SocketsHttpHandler is not supported there. I don't want to hardcode supported platforms into the client, so the only solution I see at the moment is catch PlatformNotSupportedException.

private HttpMessageHandler CreateHandler()
{
    try
    {
        var socketsHttpHandler = new SocketsHttpHandler();

        // This property will throw PlatformNotSupportedException
        socketsHttpHandler.MaxHttp2ConnectionsPerServer = int.MaxValue;

        return socketsHttpHandler;
    }
    catch (PlatformNotSupportedException)
    {
        return new HttpClientHandler();
    }
}

https://github.com/dotnet/runtime/blob/ef6c035c399ba856566a9e3f4531084c8e87496a/src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/SocketsHttpHandler.cs

This situation will only grow as more frameworks like MAUI target net5.0 and run on platforms that don't support SocketsHttpHandler.

Proposed API

A feature flag that specifies whether SocketsHttpHandler is supported.

public class SocketsHttpHandler
{
    public static bool IsSupported { get; }
}

Usage Examples

private HttpMessageHandler CreateHandler()
{
    if (SocketsHttpHandler.IsSupported)
    {
        var socketsHttpHandler = new SocketsHttpHandler();
        socketsHttpHandler.MaxHttp2ConnectionsPerServer = int.MaxValue;

        return socketsHttpHandler;
    }
    else
    {
        return new HttpClientHandler();
    }
}
@JamesNK JamesNK added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 17, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Jul 17, 2020
@ghost
Copy link

ghost commented Jul 17, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@karelz
Copy link
Member

karelz commented Jul 17, 2020

@terrajobst what is the best way to handle platform-specific API availability? Is the above API the right approach?

@scalablecory
Copy link
Contributor

This is a larger issue than just SocketsHttpHandler. A solution might tie in with the proposal at https://github.com/dotnet/designs/blob/master/accepted/2020/windows-specific-apis/windows-specific-apis.md

@scalablecory
Copy link
Contributor

@terrajobst do you have thoughts here?

@karelz
Copy link
Member

karelz commented Jul 21, 2020

Triage: Conceptually it makes sense, but we may want to use some more general approach / pattern - something for @dotnet/fxdc to discuss? Marking as ready for API review.

@karelz karelz added this to the 5.0.0 milestone Jul 21, 2020
@karelz karelz added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Jul 21, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 23, 2020
@terrajobst
Copy link
Member

terrajobst commented Jul 23, 2020

Video

We should consider a factory method like:

namespace System.Net.Http
{
    public partial class HttpMessageHandler
    {
        public static HttpMessageHandler CreateDefault();
    }
}

This would allow library code to write code like this:

private HttpMessageHandler CreateHandler()
{
    var handler = HttpMessageHandler.CreateDefault();
    if (handler is SocketsHttpHandler socketsHttpHandler)
    {
        // This property will throw PlatformNotSupportedException
        socketsHttpHandler.MaxHttp2ConnectionsPerServer = int.MaxValue;
    }

    return handler;
}

However, the proposed API also makes sense:

namespace System.Net.Http
{
    public partial class SocketsHttpHandler
    {
        public static bool IsSupported { get; }
    }
}

@scalablecory, please discuss this with your networking folks and file a separate feature, likely for 6.0.

@marek-safar
Copy link
Contributor

The method CreateDefault() is quite unfortunate as it says nothing on what the implementation does. It's also not clear what the behaviour should be when there are more than 1 handlers available. For example, on Android, we support SocketHttpHandler and AndroidHttpHandler which one is the default?

@geoffkizer
Copy link
Contributor

Per @stephentoub on the linked PR:

This needed xml doc comments.

@alnikola
Copy link
Contributor

alnikola commented Aug 4, 2020

I will add it, but would like to better understand which rules control that. Please, see my comment on the PR.

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this issue Aug 10, 2020
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this issue Aug 10, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.