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

Introduce keepalive_maxage config parameter for BaseConnector #4948

Open
adamserafini opened this issue Sep 3, 2020 · 0 comments
Open

Introduce keepalive_maxage config parameter for BaseConnector #4948

adamserafini opened this issue Sep 3, 2020 · 0 comments

Comments

@adamserafini
Copy link

🐣 Is your feature request related to a problem? Please describe.
Current keepalive_timeout parameter works like a "maximum idle time": if a connection has been idle > keepalive_timeout it gets closed. But if the connection keeps being used frequently (say, every 1s while the timeout is 75s), the connection will be kept alive indefinitely.

The problem is that we would also like to configure a maximum length of time a connection should be kept alive (since initial connection), regardless of whether it has been idle or not. For example, consider an upstream service with keep alive enabled and a short DNS TTL (60s) where we use CNAME to direct the traffic between different datacenters A and B. If the service is frequently called, the connections will be kept alive indefinitely and the traffic will never switch from A to B.

💡 Describe the solution you'd like
A new BaseConnector config parameter called keepalive_maxage that configures the maximum age in seconds a connection will be 'kept alive' before it is closed.

Implementation idea:
Currently, an individual connection is represented by a tuple(connection, timestamp_lastused)

self._conns = {} # type: Dict[ConnectionKey, List[Tuple[ResponseHandler, float]]] # noqa

We introduce a third element to this tuple (timestamp_created) which is set when the connection is made and not updated on subsequent use: tuple(connection, timestamp_lastused, timestamp_created).

To implement closing, when we check the timeout_lastused, we also check whether current time (t1) t1 - timestamp_created > keepalive_maxage and close the connection if so:

if t1 - t0 > self._keepalive_timeout:

Describe alternatives you've considered
We could call _drop_acquired_per_host every Xs ourselves to refresh but seems nicer to have this as a configuration parameter on the BaseConnector class itself.

We could subclass TCPConnector ourselves to implement this functionality but if it's useful for us, this extra config could be useful for others, hence this suggestion to implement upstream.

📋 Additional context
I (or one of my colleagues) would be more than happy to work on this feature request but I want to check the proposed feature and implementation with the maintainers before doing so 🙇

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

No branches or pull requests

1 participant