sockets: ConfigureTransport: prevent idle connections leaking FDs #134
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR sets default values for MaxIdleConns and IdleConnTimeout in the HTTP transport to prevent file descriptor leaks when idle connections persist in long-lived processes.
- Set default MaxIdleConns to 6 if not already set.
- Set IdleConnTimeout to 30 seconds to ensure idle connections are released.
| // due to idle connections not being released. | ||
| // | ||
| // TODO: see if we can also address this from the server side; see: https://github.com/moby/moby/issues/45539 | ||
| tr.MaxIdleConns = 6 |
There was a problem hiding this comment.
[nitpick] Consider extracting the magic number '6' into a named constant for improved clarity and maintainability.
| tr.MaxIdleConns = 6 | |
| tr.MaxIdleConns = defaultMaxIdleConns |
There was a problem hiding this comment.
I'll keep this for now; keeping all of these together may more clearly indicate the slightly "ad-hoc" nature of this.
| // | ||
| // TODO: see if we can also address this from the server side; see: https://github.com/moby/moby/issues/45539 | ||
| tr.MaxIdleConns = 6 | ||
| tr.IdleConnTimeout = 30 * time.Second |
There was a problem hiding this comment.
[nitpick] Consider extracting the timeout value '30 * time.Second' into a named constant to allow for easier configuration and clearer intent.
| tr.IdleConnTimeout = 30 * time.Second | |
| tr.IdleConnTimeout = idleConnTimeout |
There was a problem hiding this comment.
I'll keep this for now; keeping all of these together may more clearly indicate the slightly "ad-hoc" nature of this.
Set default `MaxIdleConns` / `IdleConnTimeout` to prevent idle connections leaking FDs. When using the client (initialized via `dockercli` in my case) in a long-running process, the idle connections are not released. If you create multiple clients the FD count keeps growing indefinitely. This can be demonstrated against Docker Desktop; other cases have not been tested, but it also depends on the server's behavior and when the server will drop the connections. Other possible fixes could be: - initialize transport from DefaultTransport (that has better defaults) - Somehow wrap transport so all clients use the same keepalive pool - Make this configurable. I would also need dockerCLI update then. - Explore server side - There is a call to `CloseIdleConns` but that lifecycle is tricky to manage when initializing clients via `dockerCLI` based on multiple configs. Possibly defining a `Finalizer` could also result in the desired behavior. This patch migrates the code from [moby@5c72a95] to this module. [moby@5c72a95]: moby/moby@5c72a95 Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
config_idlecons
branch
from
11199e1 to
dd5901a
Compare
|
/cc @austinvazquez - looks like I forgot about this one; mostly so that we can remove the same patch from moby (that said, we should probably look at removing most of this package, and have it as part of moby) |
2 participants
relates to:
Set default
MaxIdleConns/IdleConnTimeoutto prevent idle connections leaking FDs.When using the client (initialized via
dockercliin my case) in a long-running process, the idle connections are not released. If you create multiple clients the FD count keeps growing indefinitely.This can be demonstrated against Docker Desktop; other cases have not been tested, but it also depends on the server's behavior and when the server will drop the connections.
Other possible fixes could be:
CloseIdleConnsbut that lifecycle is tricky to manage when initializing clients viadockerCLIbased on multiple configs. Possibly defining aFinalizercould also result in the desired behavior.This patch migrates the code from moby@5c72a95 to this module.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)