-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Additional HTTP/2 connections created when active streams limit is reached #38748
Additional HTTP/2 connections created when active streams limit is reached #38748
Conversation
Tagging subscribers to this area: @dotnet/ncl |
Note regarding the 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. |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditManager.cs
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
- Exception string put in .resx
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/Http2LoopbackServer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditManager.cs
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Show resolved
Hide resolved
@alnikola, I've been staring at the PR and I think I'm missing something fundamental. Where are connections throttled to the max limit set? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last question.
Otherwise, apart from pending comments, LGTM.
I'll approve, once it's all cleared up. Thank you 👍
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Outdated
Show resolved
Hide resolved
It's in the Though I do wonder if in some cases, it might end up with creating more connections. Eventually throttling, but still creating some more than the setting says. |
On which connection? |
You're right, it seems on a new one. I should've known better and assume you already saw through this 😊 |
@stephentoub Throttling itself is triggered in Http2Connection.SendHeadersAsync by throwing an exception when the stream limit is reached, but the total number of connections is still below |
Where? GetHttp2ConnectionAsync calls GetHttp2ConnectionAcceptingNewStreams, which will return null if it couldn't find a connection with availability, at which point it will proceed to create a new connection. What am I missing, and where is the test that proves it? |
If |
Ok. I don't have a strong preference between the two approaches. |
By the way, what happens when a bunch of requests hit the limit at once and you need new connections, do you make one at a time and make the requests wait, or could you end up creating many at once, each with only one request? |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Show resolved
Hide resolved
Connection timeout increased to 5 sec
- Available slot search under connection construction lock is fixed - Tests execution time greatly reduced
Closing this PR for now in favor of an alternative simpler implementation #39439 |
Temporary re-open it to merge in the correct PrepareConnection implementation and make sure tests now pass. |
Closing again for now. |
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.
Fixes #35088