Skip to content
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

Provide equivalent of Agent to manage ClientHttp2Sessions across requests #6

Closed
pietermees opened this issue Sep 4, 2018 · 2 comments
Labels
enhancement New feature or request work in progress

Comments

@pietermees
Copy link

To get to a fully transparent solution, on par with Agent for h1, we should look at providing a ClientHttp2Session manager that takes on the following responsibilities:

  1. Establishing new connections when no connection is currently established or when the maximum concurrent streams has been reached on an existing ClientHttp2Session
  2. Performing ALPN negotiation so we can transparently support both h1 and h2, without knowing upfront what the remote host supports
  3. Detecting failed or closed sessions, and re-establishing them when needed
  4. Have some forms of policing, like maximum simultaneous outgoing connections, socket timeouts etc
  5. It would be great if it could also expose some metrics and statistics: how many h1 vs h2 connections, idle vs in use connections, multiplexing rate etc

We should expose both an Agent and new H2-oriented interface, so this connection pool can transparently support both protocols. This is necessary when we're connecting to a remote host and determine that it does not support h2, then we like to reuse that existing TLS connection over h1. If we expose an Agent interface`, we can reuse the TLS connection.

Ideally, this component is also DNS-aware, meaning that it could do the following:

  • If DNS resolution indicates the remote host has multiple IP addresses, re-attempt connecting to a different IP if one connection fails. This makes sure that remote outages are handled more gracefully.
  • If multiple connections need to be established to a remote host, do so across multiple of the available IPs so the load gets distributed.
@szmarczak szmarczak added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Sep 4, 2018
@szmarczak
Copy link
Owner

The DNS-related functionalities shouldn't be built-in. But, our agent class should make it possible to do. See the examples (using HTTP1 Agent):

If multiple connections need to be established to a remote host, do so across multiple of the available IPs so the load gets distributed.

https://gist.github.com/szmarczak/d29efa57d085fd7acc4e8b157f39cb45

If DNS resolution indicates the remote host has multiple IP addresses, re-attempt connecting to a different IP if one connection fails. This makes sure that remote outages are handled more gracefully.

https://gist.github.com/szmarczak/8dcf43d4c57ff952424642afe0ebada2

@szmarczak szmarczak added external This issue is related to an external project and removed help wanted Extra attention is needed labels Dec 16, 2018
@szmarczak szmarczak pinned this issue Dec 22, 2018
@szmarczak szmarczak removed the external This issue is related to an external project label Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
Repository owner deleted a comment from pietermees Apr 28, 2019
@szmarczak
Copy link
Owner

szmarczak commented Apr 28, 2019

Sorry I've deleted all of our comments but I needed to clean up this place.

I've rewritten the H2 Agent. Take a look at it (there are still some things to do, eg. correct session picking and HTTP2Agent.destroy). Note that this agent is specific for this library. Some functions may not work elsewhere.

Establishing new connections when no connection is currently established or when the maximum concurrent streams has been reached on an existing ClientHttp2Session

✔️

Performing ALPN negotiation so we can transparently support both h1 and h2, without knowing upfront what the remote host supports

✔️

Detecting failed or closed sessions, and re-establishing them when needed

We could retry failed sessions... but it's quite useless. Imagine all seats for free sockets are taken. You need to close one and make another... The retry logic should stay in a high level packages like Got.

Have some forms of policing, like maximum simultaneous outgoing connections, socket timeouts etc

✔️

It would be great if it could also expose some metrics and statistics: how many h1 vs h2 connections, idle vs in use connections, multiplexing rate etc

It won't be compatible at all with the H1 Agent. Making a compatibility layer is a hard topic. The design completely differs. I mean it's possible but it's just fixing Node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request work in progress
Projects
None yet
Development

No branches or pull requests

2 participants