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

Jump to bottom

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

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

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

go-loco opened this issue Mar 27, 2016 · 3 comments
Labels
FeatureRequest
Milestone
Unplanned

Comments

@go-loco
Copy link

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
Copy link
Contributor

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 It is not possible for clients of http.Transport to dynamically change MaxIdleConnsPerHost net/http: mechanism to dynamically change MaxIdleConnsPerHost? Mar 28, 2016
@go-loco
Copy link
Author

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
Copy link
Contributor

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

@sjlee sjlee mentioned this issue Nov 2, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FeatureRequest
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
2 participants
@bradfitz @go-loco