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

different values of MaxIdleConns and MaxIdleConnsPerHost #4201

Closed
akosyakov opened this issue Jun 11, 2021 · 3 comments
Closed

different values of MaxIdleConns and MaxIdleConnsPerHost #4201

akosyakov opened this issue Jun 11, 2021 · 3 comments
Assignees
Labels
bug 🐞 Something isn't working
Milestone

Comments

@akosyakov
Copy link

Maybe I'm missing something but it does not seem to be possible because keepalive_idle_conns always overrides max_idle_conns_per_host:

MaxIdleConnsPerHost: h.MaxIdleConnsPerHost,
ResponseHeaderTimeout: time.Duration(h.ResponseHeaderTimeout),
ExpectContinueTimeout: time.Duration(h.ExpectContinueTimeout),
MaxResponseHeaderBytes: h.MaxResponseHeaderSize,
WriteBufferSize: h.WriteBufferSize,
ReadBufferSize: h.ReadBufferSize,
}
if h.TLS != nil {
rt.TLSHandshakeTimeout = time.Duration(h.TLS.HandshakeTimeout)
var err error
rt.TLSClientConfig, err = h.TLS.MakeTLSClientConfig(ctx)
if err != nil {
return nil, fmt.Errorf("making TLS client config: %v", err)
}
}
if h.KeepAlive != nil {
dialer.KeepAlive = time.Duration(h.KeepAlive.ProbeInterval)
if h.KeepAlive.Enabled != nil {
rt.DisableKeepAlives = !*h.KeepAlive.Enabled
}
rt.MaxIdleConns = h.KeepAlive.MaxIdleConns
rt.MaxIdleConnsPerHost = h.KeepAlive.MaxIdleConnsPerHost
rt.IdleConnTimeout = time.Duration(h.KeepAlive.IdleConnTimeout)
}

@mholt
Copy link
Member

mholt commented Jun 15, 2021

Hmm that does seem like an oversight, oops.

@francislavoie What if we deprecate h.MaxIdleConnsPerHost (since it's a property of keep-alive) and remove it later, since it's a bug. (I'll take care of this.)

@mholt mholt self-assigned this Jun 15, 2021
@mholt mholt added the bug 🐞 Something isn't working label Jun 15, 2021
@mholt mholt added this to the v2.4.3 milestone Jun 15, 2021
@mholt
Copy link
Member

mholt commented Jun 15, 2021

Hm ok so here's what I'm gonna do: I'm just gonna remove h.MaxIdleConnsPerHost since currently, people's configs are silently broken if they specify both (since one overwrites the other). We could return an error if both are specified and the values are different, but that's still an error all the same as if we just remove it, and I'd rather delete code than add code, especially adding it just temporarily.

So, this could break some people's configs, but it fixes a bug that they're already experiencing anyway, so this will bring their configs in line to not be buggy anymore.

I'll also split keepalive_idle_conns into two options -- it took me a minute to figure out, by the way, that you were referring to Caddyfile subdirectives, @akosyakov -- leaving that one for just setting MaxIdleConns and then making keepalive_idle_conns_per_host to set MaxIdleConnsPerHost.

@mholt mholt closed this as completed in 7c68809 Jun 15, 2021
@mholt
Copy link
Member

mholt commented Jun 15, 2021

Dangit, forgot to run the tests before I pushed. Sigh...

mholt added a commit that referenced this issue Jun 15, 2021
@caddyserver caddyserver deleted a comment from chandrawisnu Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants