[retry only] Additional HTTP/2 connections created when active streams limit is reached #39439

alnikola · 2020-07-16T15:08:25Z

HTTP/2 standard commands clients to not open more than one HTTP/2 connection to the same server. At the same time, server has right to limit the maximum number of active streams per that HTTP/2 connection. These two directive combined impose limit on the number of requests concurrently send to the server. This limitation is justified in client to server scenarios, but become a bottleneck in server to server cases like gRPC. This PR introduces a new SocketsHttpHandler API enabling establishing additional HTTP/2 connections to the same server when the maximum stream limit is reached on the existing ones.

Note. This algorithm version uses only retries to make request choose another connection when all stream slots are occupied. It does not implement stream credit management in HttpConnectionPool and therefore exhibit a sub-optimal request scheduling behavior in "request burst" and "infinite requests" scenarios.

Fixes #35088

- Exception string put in .resx

ghost · 2020-07-16T15:08:28Z

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

Dotnet-GitSync-Bot · 2020-07-16T15:08:29Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

…oid deadlock

…after-stream-count-limit

- Tests fixed

- Debug logging for the other

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

stephentoub · 2020-07-20T16:40:01Z

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

+
+ private Http2Connection PickRandomConnection(HttpRequestMessage request, Http2Connection[] currentHttp2Connections)
+ {
+ // All connections are busy so pick a random one to wait on it.


Why did we decide this is ok? This is one of the main concerns I raised previously.

Due to a high complexity of the other approach. @geoffkizer raised the concern that other solution is so complex that it will hinder other HTTP/2 optimization efforts. We still can go back to a more advanced version later, I won't delete that another branch.

cc: @karelz

I don't think we explicitly decided it was ok. I agree, it's an issue.

The problem is, given this approach, it's hard to implement this the correct way. We don't have a notification from the Http2Connection that a new stream has become available. We could add this, of course, and we could add some queuing logic (similar to HTTP/1.1) to queue requests when all connections are busy, etc, etc.

That said, I'm not sure it's worth the trouble. The key scenario here is to support multiple HTTP2 connections. My suggestion is that we remove the connection limit here. Meaning, when you enable multiple HTTP2 connections, the number of connections is unbounded. This is what WinHTTP does as well.

That notification and queueing are implemented in my other PR #38748, but it added noticeable complexity as you can see. It still might be there is a way to achieve the same with some simpler mechanism, but I currently don't see how.

Connection notifies about an available stream via StreamSlotAvailable

Requests are queued while waiting for a free stream thanks to built-in support in SemaphoreSlim

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditManager.cs

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

geoffkizer · 2020-07-20T22:01:02Z

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

+ Http2Connection[]? localHttp2Connections = _http2Connections;
+ int newCollectionSize = localHttp2Connections == null ? 1 : localHttp2Connections.Length + 1;
+ Http2Connection[] newHttp2Connections = new Http2Connection[newCollectionSize];
+ newHttp2Connections[0] = newConnection;


Why do we add the connection at the beginning of the list? I thought we were going to add it to the end.

To have the search from the newest to the oldest in GetHttp2ConnectionAcceptingNewStreams. If we decide to change the order, we will change it here.

I guess what I'm asking is, why did we decide to search from newest to oldest instead of oldest to newest?

I don't have a strong opinion here, but I'd like to understand the rationale.

The assumption is that older connections tend to have longer running requests than new ones because the lack of available streams on the old connections is the only driver for creating new ones. Thus, it looks more efficiently to firstly check the newest for available streams.

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

alnikola · 2020-07-22T14:10:26Z

Maximum connections limit removed.

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

geoffkizer · 2020-07-23T02:45:18Z

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

@@ -648,6 +647,53 @@ public byte[] Http2AltSvcOriginUri
 return await GetHttpConnectionAsync(request, async, cancellationToken).ConfigureAwait(false);
 }

+ private Http2Connection? GetExistingHttp2Connection()
+ {
+ Http2Connection[]? localConnections = _http2Connections;


It's probably worth a comment here to explain that the array itself is never mutated, though the array reference may change.

It's explained above _http2Connections declaration.

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

src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/SocketsHttpHandler.cs

- Expiration test fixed

…g expired HTTP/2 connections - Expiration test fixed

geoffkizer · 2020-07-27T16:06:08Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

@@ -1461,7 +1473,7 @@ private void Abort(Exception abortException)

 // Check idle timeout when there are not pending requests for a while.
 if ((connectionIdleTimeout != Timeout.InfiniteTimeSpan) &&
- (_httpStreams.Count == 0) &&
+ (_httpStreams.Count == 0) && (_idleSinceTickCount > 0) &&


Why is this change necessary?

On a new Http2Connection _idleSinceTickCount is set to 0 until the first request is sent. On the other hand, setting PooledConnectionIdleTimeout to a specific value starts a timer calling this CleanCacheAndDisposeIfUnused method every second or less, so this check gets done before the first request goes through connection while _idleSinceTickCount is still 0 which is always less thanEnvironment.TickCount64 therefore a brand new connection can get almost immediately expired.

So this issue existed previously? I'm glad you caught it!

That said, maybe it would be better to initialize _idleSinceTickCount to the current tick in the Http2Connection constructor? I'm not sure where we check this value, but it seems better to have it be correct always than to special case 0.

Adding @wfurt, I think he wrote this code... thoughts?

Yes, it is an existing issue. Currently _idleSinceTickCount is read only in this method and assigned in RemoveStream. We could initialize it in the constructor, but it seems the intention was to update it when a request completes and stream gets closed, thus setting a value in the ctor will break that invariant.

The intent is to track the last time the connection might have been idle (if there were no other active streams). As such initializing it to the current tick count in the constructor seems to be more inline with the intent, to me.

I don't know if there's ever a case where we can get into a situation where an Http2Connection is created but never actually used -- maybe if cancellation happens at exactly the right time? If that ever happens, we will still want to idle the connection out eventually.

Agree, makes sense. I will initialize it in the constructor.

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

geoffkizer

Overall looks good to me. One small issue and one question above.

…s limit is reached (dotnet#39439) HTTP/2 standard commands clients to not open more than one HTTP/2 connection to the same server. At the same time, server has right to limit the maximum number of active streams per that HTTP/2 connection. These two directives combined impose limit on the number of requests concurrently send to the server. This limitation is justified in client to server scenarios, but become a bottleneck in server to server cases like gRPC. This PR introduces a new SocketsHttpHandler API enabling establishing additional HTTP/2 connections to the same server when the maximum stream limit is reached on the existing ones. **Note**. This algorithm version uses only retries to make request choose another connection when all stream slots are occupied. It does not implement stream credit management in `HttpConnectionPool` and therefore exhibit a sub-optimal request scheduling behavior in "request burst" and "infinite requests" scenarios. Fixes dotnet#35088

alnikola added 8 commits June 23, 2020 17:48

SocketsHttpHandler supports multiple HTTP/2 connections

2708f8b

Connection invalidation fixed

9852b24

Reduce test connection timeout

d97e37e

- Code duplication removed

1f0e0f2

- Exception string put in .resx

Simplified connection add and invalidate logic

687d7cd

Retry-based stream credit management fixed

f8f39fb

Merge from master

02f0b04

Remove test logging

237adaa

alnikola added area-System.Net.Http new-api-needs-documentation labels Jul 16, 2020

alnikola added this to the 5.0.0 milestone Jul 16, 2020

alnikola requested review from geoffkizer and a team July 16, 2020 15:08

alnikola added 6 commits July 17, 2020 16:48

ConfigureAwait(false) added to fix hanging tests

36bb03f

Tests run request handling code in parallel for all connections to av…

46927ac

…oid deadlock

Test execution time reduced

33ab290

Merge branch 'master' into alnikola/35088-retry-only-more-http2-conn-…

a440b76

…after-stream-count-limit

- Performance improved by optimizing all connections utilizations

99f24c8

- Tests fixed

- One hanging test fixed

30b11d8

- Debug logging for the other

alnikola mentioned this pull request Jul 20, 2020

Additional HTTP/2 connections created when active streams limit is reached #38748

Closed

Tratcher reviewed Jul 20, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs Outdated Show resolved Hide resolved

Http2Connection sets correct retry type when stream limit is reached

8846166

stephentoub reviewed Jul 20, 2020

View reviewed changes

jaredpar mentioned this pull request Jul 20, 2020

Errors installing the SDK during builds #34015

Closed

geoffkizer reviewed Jul 20, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditManager.cs Show resolved Hide resolved

geoffkizer reviewed Jul 20, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Jul 20, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

alnikola added 2 commits July 22, 2020 16:36

Comments fixed

dad85cf

PooledConnectionIdleTimeout increased to stabilize the test

8af737a

geoffkizer reviewed Jul 23, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Jul 23, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Jul 23, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Jul 23, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Jul 23, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Show resolved Hide resolved

geoffkizer reviewed Jul 23, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Jul 23, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Jul 23, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Jul 23, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/SocketsHttpHandler.cs Outdated Show resolved Hide resolved

alnikola added 5 commits July 23, 2020 11:46

- Comments addressed

307faa0

- Expiration test fixed

Merge

0640743

Test timeouts fixed

61e4297

- CleanCacheAndDisposeIfUnused allocates only one array while removin…

dda6ccd

…g expired HTTP/2 connections - Expiration test fixed

Invalid new connection expiration is fixed

c160a57

geoffkizer reviewed Jul 27, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

geoffkizer approved these changes Jul 27, 2020

View reviewed changes

Redundant check removed

67375df

jaredpar mentioned this pull request Jul 27, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

alnikola added 2 commits July 28, 2020 10:49

_idleSinceTickCount is initialized in constructor

1ddf691

Merge with master

5fc1702

alnikola merged commit 43670d5 into dotnet:master Jul 28, 2020

alnikola deleted the alnikola/35088-retry-only-more-http2-conn-after-stream-count-limit branch July 28, 2020 10:43

Tratcher mentioned this pull request Jul 31, 2020

Poor performance and errors when proxying to https microsoft/reverse-proxy#355

Closed

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[retry only] Additional HTTP/2 connections created when active streams limit is reached #39439

[retry only] Additional HTTP/2 connections created when active streams limit is reached #39439

alnikola commented Jul 16, 2020

ghost commented Jul 16, 2020

Dotnet-GitSync-Bot commented Jul 16, 2020

stephentoub Jul 20, 2020

alnikola Jul 20, 2020

alnikola Jul 20, 2020

geoffkizer Jul 20, 2020

alnikola Jul 21, 2020

alnikola Jul 21, 2020

geoffkizer Jul 20, 2020

alnikola Jul 21, 2020

geoffkizer Jul 21, 2020

alnikola Jul 21, 2020

geoffkizer Jul 21, 2020

alnikola commented Jul 22, 2020

geoffkizer Jul 23, 2020

alnikola Jul 23, 2020

geoffkizer Jul 27, 2020

alnikola Jul 27, 2020

geoffkizer Jul 27, 2020

alnikola Jul 27, 2020

geoffkizer Jul 27, 2020

alnikola Jul 28, 2020

geoffkizer left a comment

[retry only] Additional HTTP/2 connections created when active streams limit is reached #39439

[retry only] Additional HTTP/2 connections created when active streams limit is reached #39439

Conversation

alnikola commented Jul 16, 2020

ghost commented Jul 16, 2020

Dotnet-GitSync-Bot commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alnikola commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer left a comment

Choose a reason for hiding this comment