SocketsHttpHandler: Add MaxHttpVersion property

[geoffkizer]

Now that we are adding HTTP2 support in SocketsHttpHandler, we should add a setting to allow this to be easily disabled, in case customers run into problems using HTTP2 with a particular backend server.

Proposed API:

public sealed class SocketsHttpHandler
{
    public Version MaxHttpVersion { get; set; } // defaults to 2.0
    ...
}

In the short run, we will default this to 1.1 until we're ready to enable HTTP2 by default.

cc @davidsh @stephentoub @dotnet/ncl

[davidsh]

As background for the need for this, see:
https://docs.microsoft.com/en-us/uwp/api/windows.web.http.filters.httpbaseprotocolfilter.maxversion

[rmkerr]

This seems very reasonable to me.

[stephentoub]

Seems reasonable.

[karelz]

Given that it is mostly point-in-time problem, I wonder if we need new public API. Wouldn't a config switch be sufficient?
If someone wants to limit HTTP version in code, they can set HttpRequestMessage.Version I assume.

[davidsh]

If someone wants to limit HTTP version in code, they can set HttpRequestMessage.Version I assume.

This is not convenient because only HttpClient.SendAsync() takes an HttpRequestMessage object. The other APIs (used more often) are HttpClient.GetAsync(), HttpClient.PostAsync(), etc. and those don't take an HttpRequestMessage parameter at all.

[davidsh]

Given that it is mostly point-in-time problem, I wonder if we need new public API.

I would be more concerned if were talking about adding this to HttpClientHandler itself. But SocketsHttpHandler seems like a more granular API surface. And having an API for this is better than a config switch I think.

[terrajobst]

The API looks reasonable.

A few questions:

1. Why wouldn't we add the API to the base handler (HttpMessageHandler)? It seems the problem of wanting to limit the maximum HTTP version applies to all handlers.

2. Alternatively, why wouldn't we expose a instance property on HttpClient that controls the version number that it uses when constructing messages?

3. We'd also want a static property on some other type (such as HttpMessage) that controls it for the entire process.

[davidsh]

The API looks reasonable.
A few questions:

1. We don't add such properties to HttpMessageHandler. That is an abstract class that only has the .SendAsync() virtual method. We could add this property to HttpClientHandler, though. But we have been hesistant to add properties to HttpClientHandler right now. There are other properties we added to SocketsHttpHandler (i.e. PooledConnectionIdleTimeout) that we also haven't yet added to HttpClientHandler.

2. No. But we do have an HttpRequestMessage.Version property that is used for things like that. It defaults right now to HTTP/2.0.

3. There is no such thing as HttpMessage. But see 2) above.

[geoffkizer]

Some background:

The default of HttpRequestMessage.Version used to be 1.1. In the 2.1 release, we changed it to 2.0.

However, we also changed the default HttpClientHandler to use SocketsHttpHandler instead of the platform-specific handlers (WinHttpHandler and CurlHandler). SocketsHttpHandler does not support HTTP/2. Requests will automatically be downgraded to HTTP/1.1.

So when a customer upgrades from pre-2.1 to 2.1, and doesn't explicitly set the HttpRequestMessage.Version property, they will experience the following:

(1) If they use the default HttpClientHandler (implicitly or explicitly), we will still use HTTP/1.1.
(2) If they use WinHttpHandler directly, we will attempt to use HTTP/2 if the server supports it.

Note that pre-2.0, you could not use CurlHandler explicitly (you can in 2.1 via a configuration option).

Also note that HTTP/2 only applies to https requests, not plain http.

Point 2 above is a little scary: the behavior on the wire changes quite significantly. In theory, using HTTP2 just makes things better. In practice, because HTTP2 is relatively new and quite different on the wire, there may be implementation issues with particular servers or in our own client implementation that cause functional or performance problems. And because this will happen automatically on upgrade, the cause of these problems will not be obvious.

That said, this probably doesn't matter much in practice. Using WinHttpHandler directly isn't particularly common. If you do hit issues, the recommendation is to explicitly set HttpRequestMessage.Version to 1.1 on every request you issue. This is a bit of a pain, but straightforward. Or alternatively, use SocketsHttpHandler.

Now, in the 2.2 release, we are planning to implement HTTP/2 support in SocketsHttpHandler. For the reasons stated above -- particularly that the SocketsHttpHandler implementation is brand new -- we want to make HTTP2 via SocketsHttpHandler opt-in for now. That's what the MaxHttpVersion on SocketsHttpHandler does. At some point in the future (probably post-2.2) we may change the MaxHttpVersion default to 2.0, but it's still useful to have this as an opt-out if you run into problems.

This addresses the issue for SocketsHttpHandler, but doesn't help with other handlers. So potentially, we could consider moving MaxHttpVersion to HttpClient itself. This is certainly more discoverable. But it means we need to have the same default here across handlers, which is currently not the case.

I think the viable options are:

(1) Add MaxHttpVersion on SocketsHttpHandler (as proposed above)
(2) Add MaxHttpVersion on HttpClient, and default it to 1.1. This will mean anyone using WinHttpHandler or CurlHandler explicitly will need to set this property to 2.0 in order to use 2.0.

I kind of wish we had done (2) in the 2.1 release, but that ship has sailed. That said, perhaps we can still do it now; as mentioned above, I think the impact is relatively small. If so, perhaps we should consider making this change in a 2.1.x release.

[davidsh]

(2) Add MaxHttpVersion on HttpClient, and default it to 1.1. This will mean anyone using WinHttpHandler or CurlHandler explicitly will need to set this property to 2.0 in order to use 2.0.
I kind of wish we had done (2) in the 2.1 release, but that ship has sailed. That said, perhaps we can still do it now; as mentioned above, I think the impact is relatively small. If so, perhaps we should consider making this change in a 2.1.x release.

From an object-model design perspective, I am not a fan of adding these kinds of properties on HttpClient itself. HttpClient is a derived class of HttpMessageInvoker. It doesn't have these kinds of properties on it now. The intent was that HttpClientHandler was THE handler that everyone should use. It contains the actual HTTP protocol stack. If we are going to add any knobs for connections, HTTP version, etc. HttpClientHandler is the class to do that. It is similar in design to WinRT Windows.Web.Http namespace in that HttpBaseProtocolFilter is the analogous class (WinRT uses the term 'filters' instead of 'handlers').

The only reason we have SocketsHttpHandler as a public class was to experiment with adding other kinds of knobs to the handler class before we committed to adding it to HttpClientHandler. The guidance for developers in general is to use HttpClientHandler as the handler. There are other handlers out there as well but they build on top of HttpClientHandler, i.e. DelegatingHandler which takes an inner handler (usually HttpClientHandler).

So, I think for now, we go with option (1) and just add SocketsHttpHandler.MaxHttpVersion and default it to "new Version(1, 1)"

[geoffkizer]

If we are going to add any knobs for connections, HTTP version, etc. HttpClientHandler is the class to do that.

I am fine with that. That said, I think the same issues exist whether the property lives on HttpClient or HttpClientHandler.

So, I think for now, we go with option (1) and just add SocketsHttpHandler.MaxHttpVersion and default it to "new Version(1, 1)"

When you say "for now", I think you are suggesting that we can always add this property to HttpClientHandler later. Am I understanding correctly?

My concern here is mainly that we'll never revisit this. What prevents us from just deciding this today?

[davidsh]

I think you are suggesting that we can always add this property to HttpClientHandler later. Am I understanding correctly?

Yes.

My concern here is mainly that we'll never revisit this. What prevents us from just deciding this today?

@stephentoub should comment here. Several other properties were added just to SocketsHttpHandler
because it wasn't clear they would be useful long-term for the API surface. However, we could promote those to HttpClientHandler. We are cautious about that because then we have to consider what it might mean for .NET Framework when a new .NET Standard 2.x, 3.x comes out.

[stephentoub]

because it wasn't clear they would be useful long-term for the API surface

I don't think that was the reason... the main reason was that it wasn't clear they could even be implemented on an arbitrary implementation (e.g. APIs that provide fine-grained knobs around connection pooling behavior).

[geoffkizer]

Ok, let's move forward with MaxHttpVersion on SocketsHttpHandler, assuming there are no objections.

Separately, I'll file an issue for whether MaxHttpVersion should be exposed on HttpClientHandler as well.

[davidsh]

Ok, let's move forward with MaxHttpVersion on SocketsHttpHandler, assuming there are no objections.

Are you then taking this to API review meeting?

[geoffkizer]

I thought it was reviewed already? Does it need further review?

[davidsh]

I thought it was reviewed already? Does it need further review?

@terrajobst - please comment on this. This issue wasn't marked as "API approved" yet. You did say "The API looks reasonable." but then had some questions.

[geoffkizer]

I added dotnet/corefx#31207 to track HttpClientHandler.MaxHttpVersion.

[terrajobst]

Video

We've talked with @davidsh about the proposed shape and it looks good.

If we find ourselves wanting to add this property to HttpMessageHandler we can just physically move the property from SocketsHttpHandler as that's binary/source compatible. We don't need the property to be virtual.

From a usage standpoint, it seems more logical to put the property on the HttpClient as that's what vast majority of users is going to do. @davidsh's argument is that API is part of the underlying pipe which is what the handler represents. There is another problem with that design in that consumers of HttpClient can still construct an HttpRequestMessage which wouldn't/can't honor the value from HttpClient.

@bartonjs is proposing to expose HttpClient.DefaultHttpVersion (instance property) that would control what the default value for HttpRequestMessage objects will be that are constructed by HttpClient.

Any objections to this?

[davidsh]

@bartonjs is proposing to expose HttpClient.DefaultHttpVersion (instance property) that would control what the default value for HttpRequestMessage objects will be that are constructed by HttpClient.
Any objections to this?

If we do this, then we shouldn't add any property to SocketsHttpHandler, HttpMessageHandler, HttpClientHandler etc. We should simply have an HttpClient.DefaultHttpVersion property.

[geoffkizer]

@bartonjs is proposing to expose HttpClient.DefaultHttpVersion (instance property) that would control what the default value for HttpRequestMessage objects will be that are constructed by HttpClient.

I don't understand this. You don't use HttpClient to construct an HttpRequestMessage. You just do "new HttpRequestMessage".

[davidsh]

I don't understand this. You don't use HttpClient to construct an HttpRequestMessage. You just do "new HttpRequestMessage".

When developers call HttpClient APIs such as GetAsync(), PostAsync(), they only pass in a URI. However, if they use the HttpClient.SendAsync() API, then they are passing in an HttpRequestMessage explicitly.

The updated proposal is to not add a property to the handlers at all. Basically, let the HttpRequestMessage.Version property control the version. However, in cases where a developer doesn't create an HttpRequestMessage directly, then HttpClient.DefaultHttpVersion takes precedence as the value for the internally created HttpRequestMessage object and its .Version property.

[geoffkizer]

It seems really strange that doing "httpClient.GetAsync(url)" would result in a different HTTP version than "httpClient.SendAsync(new HttpRequestMessage(HttpMethod.Get, url))".

[geoffkizer]

And more generally, I'm not sure why "DefaultHttpVersion" is better than "MaxHttpVersion", regardless of where it lives.

[davidsh]

It seems really strange that doing "httpClient.GetAsync(url)" would result in a different HTTP version than "httpClient.SendAsync(new HttpRequestMessage(HttpMethod.Get, url))".

I mentioned that during the API review meeting today. But the consensus was that in the normal case, we'll have HttpClient.DefaultHttpVersion == 2.0 and HttpRequestMessage.Version == 2.0 as the initial default values of those properties. So, it would be the same in the normal use cases.

The only reason for having an HttpClient.DefaultHttpVersion property would be for devs that need to explicitly prevent HTTP/2 from being offered presumably because legacy servers can't cope with the ALPN.

[davidsh]

And more generally, I'm not sure why "DefaultHttpVersion" is better than "MaxHttpVersion", regardless of where it lives.

"DefaultHttpVersion" is better than "MaxHttpVersion" if we're adding a property to HttpClient. That is because it is controlling the "default" version of an object (HttpRequestMessage) that devs are not creating themselves (due to using the convenience APIs like GetAsync, PostAsync).

[geoffkizer]

But the consensus was that in the normal case, we'll have HttpClient.DefaultHttpVersion == 2.0 and HttpRequestMessage.Version == 2.0.

How is this the normal case?

We want to make HTTP1.1 the default when using SocketsHttpHandler. How does that work in this proposal?

Perhaps it's also worth considering the following future cases:

(1) If/when we want to make HTTP2 the default for SocketsHttpHandler
(2) If/when we implement some future HTTP version, like HTTP2.1/HTTP3.0, but want to make it opt-in
(3) If/when we later decide to make HTTP2.1/HTTP3.0 the default

I think part of the pain here is that we didn't work through all these sorts of scenarios previously. Let's not repeat that mistake now.

[davidsh]

We want to make HTTP1.1 the default when using SocketsHttpHandler. How does that work in this proposal?

I don't think we necessarily want HTTP/1.1 as the default for SocketsHttpHandler. I think we considered this as a backup plan in case we're not confident of the completeness of SocketsHttpHandler for the next release. But in general, I think we would want the default to be HTTP/2.0 to match the previous release (.Net Core 2.0) which supported HTTP/2.0 as well.

Perhaps it's also worth considering the following future cases:

How do you see handling those cases given the APIs we have now (HttpRequestMessage.Version and the current default value of 2.0)?

[geoffkizer]

But the consensus was that in the normal case, we'll have HttpClient.DefaultHttpVersion == 2.0 and HttpRequestMessage.Version == 2.0. So, it would be the same in the normal use cases.

I also think it's really weird to say "it works fine in the normal case" when discussing how to design this property. Of course it works fine in the normal case -- if not, it's horribly broken. The reason to have a property here is so that you can change behavior -- i.e., set DefaultHttpVersion to 1.1 (or in the future, some value less than the max supported version). If this applies to GetAsync but not SendAsync, why is that good? If I'm setting the propery to 1.1, presumably I only want to use 1.1. If I want to control it per request (unusual, but possible) I can explicitly set it per-request.

[geoffkizer]

Just to make sure I understand where we are at a high-level here:

Are we or are we not approved on SocketsHttpHandler.MaxHttpVersion? Can we proceed here and discuss the HttpClient/HttpClientHandler setting in parallel? Or do we think we can't settle that until we have clarity on the HttpClient/HttpClientHandler setting?

@terrajobst

[davidsh]

We are not approved on the API. That is why @terrajobst changed it back to "api-needs-work".

There was strong pushback against adding a SocketsHttpHandler level API itself.

There would be more support for adding an HttpClient.MaxHttpVersion property and setting that default to something appropriate.

[geoffkizer]

So, we should get consensus on the shape of the API. We can then decide later what the default is...or even start with HTTP/1.1 and switch (before ship) to HTTP/2.0.

I agree, but as I said above, we need an API that enables us to make HTTP2 opt-in, at least for SocketsHttpHandler.

[davidsh]

I agree, but as I said above, we need an API that enables us to make HTTP2 opt-in, at least for SocketsHttpHandler.

I think having HttpClient.MaxHttpVersion would give us that even if the default value is 1.1, we can set it to 2.0.

[geoffkizer]

I think having HttpClient.MaxHttpVersion would give us that even if the default value is 1.1, we can set it to 2.0.

Yes, I agree.

[davidsh]

Another thing we should consider if how this new property HttpClient.MaxHttpVersion will affect the current platform handlers (CurlHandler and WinHttpHandler). Right now, their "max version" is 2.0.

Adding this new property with a default value of HTTP/1.1 could change that behavior assuming we "wire it" into the code logic to pass in the new property value to those handlers. Customers that would use .NET Core and revert back to using the platform handlers (or even WinHttpHandler specifically since it is a public API) would have to change their code to adjust for the new default max version behavior.

This would also affect existing tests that we run against the platform handlers.

What should we do regarding this?

[geoffkizer]

Another thing we should consider if how this new property HttpClient.MaxHttpVersion will affect the current platform handlers (CurlHandler and WinHttpHandler). Right now, their "max version" is 2.0.

I agree. This is a reason to not have the property on HttpClient.

That said, I don't think it's much of a problem if we have the property on HttpClientHandler, and default it to 1.1. This won't affect customers who use WinHttpHandler directly. It will affect customers who switch to the platform handler in app settings. But (a) I don't think that's too common, and (b) it's arguably weird that changing the app setting changes your HTTP version today, so I think we can make a case that we're correcting this weirdness. Thoughts?

[davidsh]

I agree. This is a reason to not have the property on HttpClient.

In the API review meeting today there was no discussion around the platform handlers and possible regression in behaviors.

I think these trade-offs will need to be discussed at the next API review meeting once we revise this current proposal and re-submit it for "api-review".

[geoffkizer]

Yes, I agree.

Personally, I lean toward putting this on HttpClientHandler. It seems like the most reasonable tradeoff between usability and minimizing behavioral changes, and it's consistent with the existing model where most settings live on the HttpClientHandler.

I don't really understand the push to have this on HttpClient instead; customers already know how to use HttpClientHandler for these kinds of settings.

That said, I could live with having this on HttpClient if there's consensus for that.

[geoffkizer]

And to be clear, by "this" I mean MaxHttpVersion, not DefaultHttpVersion, which seems broken to me.

[davidsh]

All of these proposals have the drawback that there either needs to be a property on the underlying object (handler) regardless to adjust the version property, or we have to modify the input argument HttpRequestMessage's Version property before that underlying handler uses it.

My observations from the API review meeting today suggested to me that the preference is to arrive at a final API shape and not add "interim" APIs which gives us debt to fix later.

Our next step is to revise this API proposal, mark it as "api-ready-for-review" and discuss this in-person at the next meeting.

[geoffkizer]

Our next step is to revise this API proposal, mark it as "api-ready-for-review" and discuss this in-person at the next meeting.

Who is driving this? Me? You?

[bartonjs]

@geoffkizer If someone is saying new HttpRequestMessage themselves and they encounter an error with HTTP/2 the solution is easy: set the Version property to 1.1.

If someone is just using HttpClient.PostAsync(Uri, ...) they never see the HttpRequestMessage, and my suggestion is that they be able to control that message factory (HttpClient).

I don't really understand the push to have this on HttpClient instead; customers already know how to use HttpClientHandler for these kinds of settings.

Whenever I run across users of HttpClient on StackOverflow they don't seem to know what the handler is, or why they need to change from the default ctor to this other one. HttpClient..ctor vs HttpClient..ctor(HttpMessageHandler) says that by the distinct callers the default ctor is used about twice as often. This suggests that if a user is having problem with "the HttpClient they're using" and "this server" that they want a way to fix the problem with the client, not the handler.

Similarly, SendAsync(HttpRequestMessage) (without the cancellation token :frown:) is the most popular API on HttpClient which takes a pre-built message. PostAsync(String,HttpContent) is about 30% more popular, and GetAsync(String) is about 20% more popular (as the most popular overloads in their families). So it's charitably "half" of users who build their own HttpRequestMessage, but I'd say it's more skewed toward users using the Uri/String convenience methods, 5:4 or stronger.

So, the pushback is that the average customer isn't touching the handler object, and putting it on the client would allow for a one line change of the average customer's code to deal with a problem.

If you believe that the maximum (either in "throw if above this" or "downgrade if above this") makes sense by itself, then the recommendation is to put it where you'd like it to live longer term (HttpClientHandler or HttpMessageHandler, for example) instead of on SocketsHttpHandler. If the whole thing is for "we want this to be opt-in for one version" then I'd suggest something like an AppContext setting over public API.

[davidsh]

then the recommendation is to put it where you'd like it to live longer term (HttpClientHandler or HttpMessageHandler, for example) instead of on SocketsHttpHandler.

The reality is that we have to put it on both HttpClientHandler and SocketsHttpHandler since that is the way that HttpClientHandler is implemented today (as a wrapper around SocketsHttpHandler).

[davidsh]

If the whole thing is for "we want this to be opt-in for one version" then I'd suggest something like an AppContext setting over public API.

Perhaps we should consider this approach since it would allow us to use an AppContext and/or environment variable to toggle SocketsHttpHandler behavior without having to touch the existing platform handlers' behavior.

[geoffkizer]

@bartonjs Thanks for the data, that's very useful.

I can't quite tell if you're advocating for DefaultHttpVersion vs MaxHttpVersion or not. Are you?

Whenever I run across users of HttpClient on StackOverflow they don't seem to know what the handler is, or why they need to change from the default ctor to this other one.

Yeah, I'm not terribly surprised by this. I think the current split is unfortunate. But it exists. In order to do auth, compression, client certs, ssl options, or manage cookies and redirect settings, you need to use HttpClientHandler. Should these be promoted to HttpClient as well? Is MaxHttpVersion important enough to justify having it on HttpClient if the others aren't? I don't think we expect it to be used all that commonly; it's more of an escape valve than anything. Given that putting it on HttpClient introduces some additional complexity, I just don't think it's worth it.

the recommendation is to put it where you'd like it to live longer term (HttpClientHandler or HttpMessageHandler, for example) instead of on SocketsHttpHandler

Yes, I get this and agree. As I said above, I'd be happy to see this on HttpClientHandler. The main point of contention seems to be HttpClient vs HttpClientHandler.

[geoffkizer]

If the whole thing is for "we want this to be opt-in for one version" then I'd suggest something like an AppContext setting over public API.

Perhaps we should consider this approach since it would allow us to use an AppContext and/or environment variable to toggle SocketsHttpHandler behavior without having to touch the existing platform handlers' behavior.

Just to back up for a second, there are two (possible) goals here:

(1) Allow customers to opt-in to HTTP2 with SocketsHttpHandler, until we decide to make it the default (in the current release or future)
(2) Make it easy for customers to turn off HTTP2 if they hit functional/perf issues due to client or server implementation problems

The proposal here was to do (1) via a property on SocketsHttpHandler, and consider separately whether we want to do something about (2) -- hence dotnet/corefx#31207.

The main pushback, as I understand it, is that we should actually be solving the more general problem of (2). Am I misunderstanding this? If we don't think (2) is important, then why are we spending time on it?

[bartonjs]

I can't quite tell if you're advocating for DefaultHttpVersion vs MaxHttpVersion or not.

I prefer Default, and letting the users who create their own HttpRequestMessage control their destiny.

I don't think we expect it to be used all that commonly; it's more of an escape valve than anything. ...
The main point of contention seems to be HttpClient vs HttpClientHandler.

I think that the feature was being proposed as "we're afraid of our implementation". I think that if we look at it as both "ours could be wrong" and "some random server's / load balancer's could be wrong" then HttpClient makes more sense (to me), because it applies better to both situations. (Particularly given my personal feeling about the Default vs Max).

The reason that I think this one especially fits on HttpClient is that I'm suggesting that the thing that fits the user scenario best (as I understand it) is to change the HttpRequestMessage factory within HttpClient, and not to change the handlers or user-created messages. So my suggestion is just to change how the convenience methods work.

In order to do auth, compression, client certs, ssl options, or manage cookies and redirect settings, you need to use HttpClientHandler. Should these be promoted to HttpClient as well?

I think it makes sense to investigate some of those based on how our users seem to view the API. Certainly constructor-time options are hard to expose. But users don't seem to understand the intended design, so I think it's worth evaluating whether it's better overall to modify the design, or figure out how to get the users to better understand it. Again, as a separate point.

[bartonjs]

@geoffkizer If the goal of this proposal is "Allow customers to opt-in to HTTP2 with SocketsHttpHandler, until we decide to make it the default (in the current release or future)" then I don't think that should be permanent, public API. That sounds like something more ephemeral, like an environment variable or AppContext-based setting.

I'd even recommend putting a version number in it, so it's clear that in vNextNext it's no longer used. Switch.System.Net.Http.HttpClient.netcoreapp30.UseHttp2WhenAvailable, or something.

[geoffkizer]

@geoffkizer If the goal of this proposal is "Allow customers to opt-in to HTTP2 with SocketsHttpHandler, until we decide to make it the default (in the current release or future)" then I don't think that should be permanent, public API. That sounds like something more ephemeral, like an environment variable or AppContext-based setting.

If that's the recommendation, then I'm fine with that. It's a bit more painful for our testing, but that's not really relevant.

[geoffkizer]

Do AppContext settings and/or env vars require API approval?

[bartonjs]

Do AppContext settings and/or env vars require API approval?

Nope, they're not API 😄.

[terrajobst]

Fair, although I'd say everything involving the developer experience should be reviewed by someone 😄

[davidsh]

Closing this in favor of dotnet/corefx#31424.