Skip to content

HttpConnectionPool should aggressively open multiple HTTP/2 connections under load if EnableMultipleHttp2Connections is set #66944

New issue
New issue
Open
Open
HttpConnectionPool should aggressively open multiple HTTP/2 connections under load if EnableMultipleHttp2Connections is set#66944
Labels
area-System.Net.HttpenhancementProduct code improvement that does NOT require public API changes/additions
Milestone
Future

Description

@MihaZupan
MihaZupan
opened on Mar 21, 2022

On HTTP/1.1, the HttpConnectionPool will attempt to open as many connections as there are pending requests (within the limits of MaxConnectionsPerServer).

On HTTP/2, we will only open one connection at a time, even if we have a lot of requests pending and EnableMultipleHttp2Connections is set.

We should consider attempting to open up to Number of requests / InitialMaxConcurrentStreams (100) connections if EnableMultipleHttp2Connections is set and existing connections aren't able to support more streams.

Activity

  1. MihaZupan
    added
    area-System.Net.Http
    on Mar 21, 2022
  2. ghost added
    untriagedNew issue has not been triaged by the area owner
    on Mar 21, 2022

  4. wfurt commented on Mar 21, 2022

    @wfurt
    wfurt
    on Mar 21, 2022
    Member

    This would effectively violate H/2 RFC recommendation

    https://datatracker.ietf.org/doc/html/rfc7540#section-9.1

       Clients SHOULD NOT open more than one HTTP/2 connection to a given
   host and port pair, where the host is derived from a URI, a selected
   alternative service [[ALT-SVC](https://datatracker.ietf.org/doc/html/rfc7540#ref-ALT-SVC)], or a configured proxy.

  5. MihaZupan commented on Mar 21, 2022

    @MihaZupan
    MihaZupan
    on Mar 21, 2022
    MemberAuthor

    Right, but isn't that the point behind EnableMultipleHttp2Connections's existence?

  6. wfurt commented on Mar 21, 2022

    @wfurt
    wfurt
    on Mar 21, 2022
    Member

    I assumed we would but that contradicts the "is set and existing connections aren't able to support more streams."

  7. wfurt
    changed the title [-]HttpConnectionPool should aggressively open multiple HTTP/2 connections under load[/-] [+]HttpConnectionPool should aggressively open multiple HTTP/2 connections under load if EnableMultipleHttp2Connections is set[/+] on Mar 21, 2022

  8. karelz commented on Mar 22, 2022

    @karelz
    karelz
    on Mar 22, 2022
    Member

    Triage:

    • We are already violating RFC as soon as user sets EnableMultipleHttp2Connections
    • @ManickaP wants to check RFC if there is note for running out of streams
    • Current proposal has risk to have more connections than strictly needed
    • It is designed for bursty traffic

  9. ManickaP commented on Mar 22, 2022

    @ManickaP
    ManickaP
    on Mar 22, 2022
    Member

    I misread this part:

    A client can create additional connections as replacements, either to
    replace connections that are near to exhausting the available stream
    identifier space (Section 5.1.1)

    which is about running out of integer values for stream id and not running out of available streams on a connection.

    For completeness, the original issue: #35088 which references 2 prior arts:

    And only WinHTTP docs mention opening a new connection after stream limit is hit:

    Allows new requests to open an additional HTTP/2 connection when the maximum concurrent stream limit is reached, rather than waiting for the next available stream on an existing connection.

    So I'm not against opening connections more aggressively if we take into account the max concurrent streams count.

    cc: @Tratcher @JamesNK

  10. halter73 commented on Mar 23, 2022

    @halter73
    halter73
    on Mar 23, 2022
    Member

    I think attempting to open Number of requests / SETTINGS_MAX_CONCURRENT_STREAMS might make sense after some initial delay to ensure the number of concurrent requests doesn't quickly decrease after a burst. EnableMultipleHttp2Connections being set probably is an indication that the caller expects many requests to be long running and not bandwidth constrained therefore requiring a lot of concurrency.

    I'm less sure about Number of requests / InitialMaxConcurrentStreams (100). I think HttpClient should probably wait for at least one SETTINGS frame before opening too many connections.

  11. MihaZupan commented on Mar 23, 2022

    @MihaZupan
    MihaZupan
    on Mar 23, 2022
    MemberAuthor

    I suggested InitialMaxConcurrentStreams as some starting point, but we could go with some factor of that.

    The logic could be:

    • Start out with MaxStreamsPerPendingConnection = 5 * InitialMaxConcurrentStreams (500)
    • The check when injecting connections is queueCount > MaxStreamsPerPendingConnection * pendingConnections
    • When a connection is established, the MaxStreamsPerPendingConnection is updated to SETTINGS_MAX_CONCURRENT_STREAMS * 5

    Alternatively, we could start with MaxStreamsPerPendingConnection = int.MaxValue and force waiting for at least one established connection.

    This would still be rather conservative in avoiding opening too many connections while doing something if the queue grows.

  12. JamesNK commented on Mar 23, 2022

    @JamesNK
    JamesNK
    on Mar 23, 2022
    Member

    What is the goal here? Improve performance by spreading HTTP/2 requests across more HTTP connections?

  13. JamesNK commented on Mar 23, 2022

    @JamesNK
    JamesNK
    on Mar 23, 2022
    Member

    This would effectively violate H/2 RFC recommendation

    datatracker.ietf.org/doc/html/rfc7540#section-9.1

       Clients SHOULD NOT open more than one HTTP/2 connection to a given
   host and port pair, where the host is derived from a URI, a selected
   alternative service [[ALT-SVC](https://datatracker.ietf.org/doc/html/rfc7540#ref-ALT-SVC)], or a configured proxy.

    I remember this discussion from when EnableMultipleHttp2Connections was added. I'm in the firm belief that the spec is worded from a browser perspective.

    The requirement makes sense when there are many clients, e.g. browsers, making requests from a server. It doesn't make sense when there is a single client, e.g. a microservice app or proxy, making requests from a server.

    My 2 cents.

  14. MihaZupan commented on Mar 23, 2022

    @MihaZupan
    MihaZupan
    on Mar 23, 2022
    MemberAuthor

    What is the goal here?

    Increase the speed with which we ramp up the number of connections given a burst of requests.

    It also helps in the case where opening a connection may be slow / sometimes fail, by having multiple pending attempts at a time.

  15. JamesNK commented on Mar 23, 2022

    @JamesNK
    JamesNK
    on Mar 23, 2022
    Member

    Ok.

    Why does the proposed InitialMaxConcurrentStreams start with Initial? I remember there used to be a bug in SocketsHttpHandler where it wouldn't limit the number of streams it started on a connection until it got SETTINGS_MAX_CONCURRENT_STREAMS from the server. A sudden burst of requests would cause more than 100 streams which caused the server to start rejecting them. The fix was to have an artificial limit of 100 until the server settings were received. Is this property value for this time? And then it gets overridden by the server's SETTINGS_MAX_CONCURRENT_STREAMS.

    I think the property should drop Initial and just be MaxConcurrentStreams. If the goal is to distribute streams over more connections then a client setting that specifies a limit to the number of streams on a connection is fine. A client can't exceed SETTINGS_MAX_CONCURRENT_STREAMS but there is nothing stopping it from sending fewer streams on a connection before opening a new one.

    int serverMaxConcurrentStreams = GetFromSettings();
int maxStreams = Math.Min(serverMaxConcurrentStreams, socketsHttpHandler.MaxConcurrentStreams);

    So a server specifies SETTINGS_MAX_CONCURRENT_STREAMS = 100, but SocketsHttpHandler.MaxConcurrentStreams = 20, so the client sends up to a maximum of 20 requests on a connection.

  16. MihaZupan commented on Mar 23, 2022

    @MihaZupan
    MihaZupan
    on Mar 23, 2022
    MemberAuthor

    InitialMaxConcurrentStreams is the already existing limit of 100 streams until we receive the settings. It's an internal constant and not configurable at the moment.

    If the goal is to distribute streams over more connections

    This issue specifically isn't about allowing more/less streams per connection.
    It's only about allowing having more than 1 pending connection attempt in flight at a time.

    If your connections are established quickly enough, there would be no real visible difference in behavior.

    Having such a property may be interesting, but that's a separate issue.

  17. JamesNK commented on Mar 23, 2022

    @JamesNK
    JamesNK
    on Mar 23, 2022
    Member

    Ah ok. I thought part of the issue was to add a new property called InitialMaxConcurrentStreams.

    re: "Current proposal has risk to have more connections than strictly needed" - I think this is fine. The initial burst may create more connections than are needed, but the logic SocketsHttpHandler uses to assign new requests favors the first connections created. Any extra connections created during the initial burst will run out of work and then be closed for inactivity.

  18. karelz commented on Mar 29, 2022

    @karelz
    karelz
    on Mar 29, 2022
    Member

    Triage: Looks like we are ok with current proposal. RFC is not a problem anymore. More connections is alright in this case and can be capped if needed later.

    Not critical -> Future

  19. karelz
    added this to the Future milestone on Mar 29, 2022
  20. karelz
    added
    enhancementProduct code improvement that does NOT require public API changes/additions
    and removed
    untriagedNew issue has not been triaged by the area owner
    on Mar 29, 2022

  21. vasicvuk commented on May 24, 2022

    @vasicvuk
    vasicvuk
    on May 24, 2022

    For server-side cases definitely it makes sense to have a configurable number of connections open. This way the load will be spread if you have multiple instances of some microservice or scaled app.

    What about having Min and Max pool sizes as options (MinConnectionsPerServer, MaxConnectionsPerServer)?

    Then at the start, it will open MinConnectionsPerServer and round-robin through these connections, if some connection reaches MaxConcurrentStreams then it could open another one up until MaxConnectionsPerServer are open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area-System.Net.HttpenhancementProduct code improvement that does NOT require public API changes/additions

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions