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

net/http: mechanism to dynamically change MaxIdleConnsPerHost? #14984

Open
go-loco opened this Issue Mar 27, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@go-loco

go-loco commented Mar 27, 2016

Please answer these questions before submitting your issue. Thanks!

  • What version of Go are you using (go version)?
1.6
  • What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/hernan/apps/go"
GORACE=""
GOROOT="/Users/hernan/apps/go/go-src/go"
GOTOOLDIR="/Users/hernan/apps/go/go-src/go/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/vw/ht0lksb16l199rbp98p7hhfr0000gn/T/go-build001484838=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
  • What did you do?

It is not possible for clients of http.Transport to dynamically change MaxIdleConnsPerHost attribute. It would be great to have a sync.RWMutex as an attribute of http.Transport, so this is possible.

The use case is that for HTTP/1.1 Hosts, it would be nice for the client to have the possibility of adjusting max-idle connections base on usage.

The following gist shows the problem:
https://gist.github.com/go-loco/810fe53d8aee8879b8d1

Making the following changes to http.Transport would add this feature:

    // MaxIdleConnsPerHost, if non-zero, controls the maximum idle
    // (keep-alive) connections to keep per-host. If zero,
    // DefaultMaxIdleConnsPerHost is used.
    MaxIdleConnsPerHost int

    // MaxIdleConnsMutex, let you dynamically change the
    // MaxIdleConnsPerHost without creating a data race.
    //
    // Usage:
    //
    //  MaxIdleConnsMutex.Lock()
    //  MaxIdleConnsPerHost = newValue
    //  MaxIdleConnsMutex.Unlock()
    //
    MaxIdleConnsMutex sync.RWMutex
func (t *Transport) tryPutIdleConn(pconn *persistConn) error {

    t.MaxIdleConnsMutex.RLock()
    max := t.MaxIdleConnsPerHost
    t.MaxIdleConnsMutex.RUnlock()

    if t.DisableKeepAlives || max < 0 {
        return errKeepAlivesDisabled
    }
    if pconn.isBroken() {
        return errConnBroken
    }
    key := pconn.cacheKey

    if max == 0 {
        max = DefaultMaxIdleConnsPerHost
    }
...
@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 28, 2016

Exposing a RWMutex is weird. There's no precedent for that. If anything it would be a setter method, and/or a pluggable conn pool mechanism for others to do whatever they want.

@bradfitz bradfitz added this to the Unplanned milestone Mar 28, 2016

@bradfitz bradfitz changed the title from It is not possible for clients of http.Transport to dynamically change MaxIdleConnsPerHost to net/http: mechanism to dynamically change MaxIdleConnsPerHost? Mar 28, 2016

@go-loco

This comment has been minimized.

go-loco commented Mar 28, 2016

I completely agree.

A better solution would be, as you describe, adding the following method to http.Transport:

func (t *Transport) SetMaxIdleConnections(maxIdleConns int) {

    t.idleMu.Lock()
    defer t.idleMu.Unlock()

    t.MaxIdleConnsPerHost = maxIdleConns
}

and changing tryPutIdleConn

func (t *Transport) tryPutIdleConn(pconn *persistConn) error {

    t.idleMu.Lock()
    defer t.idleMu.Unlock()

    max := t.MaxIdleConnsPerHost

    if t.DisableKeepAlives || max < 0 {
        return errKeepAlivesDisabled
    }
    if pconn.isBroken() {
        return errConnBroken
    }
    key := pconn.cacheKey

    if max == 0 {
        max = DefaultMaxIdleConnsPerHost
    }
    pconn.markReused()

    waitingDialer := t.idleConnCh[key]
...
}
@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 28, 2016

Related: #13801 and #13957 and #12580 and #12289

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