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

Limit maximum size of connection pool #77

Closed
jsha opened this issue Jun 21, 2020 · 2 comments · Fixed by #81
Closed

Limit maximum size of connection pool #77

jsha opened this issue Jun 21, 2020 · 2 comments · Fixed by #81

Comments

@jsha
Copy link
Collaborator

jsha commented Jun 21, 2020

Right now, connections stay in the pool indefinitely. If someone makes requests to a wide variety of hosts, that can quickly fill up the pool. Each entry in the pool uses an FD, and eventually a program will hit ulimit_nofile and stop working.

We should set a max size for the pool, and when a new connection needs to be added but the pool is full, remove one of the existing connections. A couple of options for how to remove:

  • We could scan the pool for connections where the server has already closed them. Such connections are taking up resources needlessly. But if the connection pool is full in steady state, this could be a lot of scanning.
  • We could use an LRU cache.
  • We could remove an entry at random.
@algesten
Copy link
Owner

What is a reasonable pool size? I think the size to some extent could guide the strategy.

@jsha
Copy link
Collaborator Author

jsha commented Jun 21, 2020

On Linux systems, 1024 is a common default for ulimit_nofile, so we want to be below that, with some headroom for other tasks the program needs to do that use up fds. Go defaults to a MaxIdleConns of 100, which seems to work fine; let's do 100.

jsha added a commit to jsha/ureq that referenced this issue Jun 22, 2020
Adds limit of 100 connections to ConnectionPool. This is implemented
with a VecDeque acting as an LRU. When the pool becomes full, the oldest
stream is popped off the back from the VecDeque and also removed from
the map of PoolKeys to Streams.

Fixes algesten#77
jsha added a commit to jsha/ureq that referenced this issue Jun 23, 2020
Adds limit of 100 connections to ConnectionPool. This is implemented
with a VecDeque acting as an LRU. When the pool becomes full, the oldest
stream is popped off the back from the VecDeque and also removed from
the map of PoolKeys to Streams.

Fixes algesten#77
@jsha jsha closed this as completed in #81 Jun 24, 2020
jsha added a commit that referenced this issue Jun 24, 2020
Adds limit of 100 connections to ConnectionPool. This is implemented
with a VecDeque acting as an LRU. When the pool becomes full, the oldest
stream is popped off the back from the VecDeque and also removed from
the map of PoolKeys to Streams.

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

Successfully merging a pull request may close this issue.

2 participants