Removing extraneous call to IHttpClient.Initialize #3830

Merged
merged 1 commit into from Dec 22, 2016

Projects

None yet

3 participants

@moozzyk
Contributor
moozzyk commented Dec 14, 2016

An extraneous call to IHttpClient.Initialize when sending the start requests resets httpClients in the DefaultHttpClient. This may break load balancers and prevent reusing HTTPS connections (SSL negotation will happen twice - once for negotiate and once for start)

More: #3829

@moozzyk moozzyk Removing extraneous call to IHttpClient.Initialize
An extraneous call to IHttpClient.Initialize when sending the start requests resets httpClients in the DefaultHttpClient. This may break load balancers and prevent reusing HTTPS connections (SSL negotation will happen twice - once for negotiate and once for start)

More: #3829
ecda163
@moozzyk
Contributor
moozzyk commented Dec 14, 2016
@@ -57,8 +57,6 @@ public virtual Task<string> GetStartResponse(IHttpClient httpClient, IConnection
var startUrl = UrlBuilder.BuildStart(connection, transport, connectionData);
- httpClient.Initialize(connection);
@halter73
halter73 Dec 20, 2016 Member

It seems kinda fragile depending on GetNegotiationResponse to be called immediately before GetStartResponse on the same IHttpClient. I don't think either of the Get* methods should call Initialize if they both can't.

We should call Initialize in Connection.Start(IHttpClient httpClient) instead.

@moozzyk
moozzyk Dec 20, 2016 Contributor

To send the start request you need the connectionToken token you receive in the response to the negotiate request. Given that, this logic is hardcoded in the client and not something a user can control I think we can rely on the fact that negotiate will be called right before start.
Initializing the httpClient when sending the start does not fully fix the issue since in this case the negotiate request will use a different http client than start request potentially resulting in sending requests to different servers and forcing separate https handshakes.

@halter73
halter73 Dec 20, 2016 Member

I understand the desire to remove the double initialize given the .NET 4.5 version of DefaultHttpClient.

It's more difficult than I first realized to move the call to Initialize outside of TransportHelper. So I would just :shipit:

@moozzyk
moozzyk Dec 22, 2016 Contributor

Yeah, it would be much bigger change.

@moozzyk moozzyk merged commit 0b362f6 into dev Dec 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment