Skip to content

Commit

Permalink
Update DefaultTransport with a better MaxIdleConnsPerHost and newer f…
Browse files Browse the repository at this point in the history 
…ields
  • Loading branch information
@jefferai
jefferai committed Feb 11, 2017
1 parent ad28ea4 commit a459706
Showing 1 changed file with 8 additions and 5 deletions.
13 changes: 8 additions & 5 deletions cleanhttp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cleanhttp
import (
"net"
"net/http"
"runtime"
"time"
)

Expand All @@ -22,13 +23,15 @@ func DefaultTransport() *http.Transport {
func DefaultPooledTransport() *http.Transport {
transport := &http.Transport{
Proxy: http.ProxyFromEnvironment,
Dial: (&net.Dialer{
DialContext: (&net.Dialer{
Timeout: 30 * time.Second,
KeepAlive: 30 * time.Second,
}).Dial,
TLSHandshakeTimeout: 10 * time.Second,
DisableKeepAlives: false,
MaxIdleConnsPerHost: 1,
}).DialContext,
MaxIdleConns: 100,
IdleConnTimeout: 90 * time.Second,
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 1 * time.Second,
MaxIdleConnsPerHost: runtime.GOMAXPROCS(0) + 1,
}
return transport
}
Expand Down

10 comments on commit a459706

@bluewhale
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit has broken the library in golang 1.6 ubuntu 16.04.

INFO[0328] # github.com/hashicorp/go-cleanhttp
INFO[0328] ../github.com/hashicorp/go-cleanhttp/cleanhttp.go:29: unknown http.Transport field 'DialContext' in struct literal
INFO[0328] ../github.com/hashicorp/go-cleanhttp/cleanhttp.go:30: unknown http.Transport field 'MaxIdleConns' in struct literal
INFO[0328] ../github.com/hashicorp/go-cleanhttp/cleanhttp.go:31: unknown http.Transport field 'IdleConnTimeout' in struct literal

@jefferai
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go 1.8 is just about out. Per https://golang.org/doc/devel/release.html Go 1.6 is already officially unsupported; with the release of Go 1.8 it will be unsupported even for critical security bugs. I suggest upgrading.

@tahmmee
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jefferai, this is also a major problem for upstream projects. Is there anyway to offer a backward compatible solution? I'm guessing more people will continue to fork and revert this which is unfortunate.

@jefferai
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tahmee build tags can select for go version. If someone were to PR that I'd be happy to merge. But I don't have interest/time in doing that myself given that 1.6 is explicitly unsupported within a couple of days.

@richard-mauri
Copy link

@richard-mauri richard-mauri commented on a459706 Feb 16, 2017 •
edited by sethvargo
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My project using go1.7.4 is now busted. It does a go get of hashi vault packages and that fails before it does a git checkout 0.6.4 (crude vendoring to back to that tag)

> vagrant@vaultaccess-host:~/vaultaccess$ go version
> go version go1.7.4 linux/amd64
> vagrant@vaultaccess-host:~/vaultaccess$ 
> vagrant@vaultaccess-host:~/vaultaccess$ ./build.sh 
> # github.com/hashicorp/vault/vendor/github.com/hashicorp/go-cleanhttp
> ../github.com/hashicorp/vault/vendor/github.com/hashicorp/go-cleanhttp/cleanhttp.go:29: unknown http.Transport field 'DialContext' in struct literal
> ../github.com/hashicorp/vault/vendor/github.com/hashicorp/go-cleanhttp/cleanhttp.go:30: unknown http.Transport field 'MaxIdleConns' in struct literal
> ../github.com/hashicorp/vault/vendor/github.com/hashicorp/go-cleanhttp/cleanhttp.go:31: unknown http.Transport field 'IdleConnTimeout' in struct literal
> Note: checking out 'v0.6.4'.
> 
> You are in 'detached HEAD' state. You can look around, make experimental
> changes and commit them, and you can discard any commits you make in this
> state without impacting any branches by performing another checkout.
> 
> If you want to create a new branch to retain commits you create, you may
> do so (now or later) by using -b with the checkout command again. Example:
> 
>   git checkout -b new_branch_name
> 
> HEAD is now at f4adc7f... Cut version 0.6.4

@jefferai
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richard-mauri Something is odd with your Go installation. Everything in your errors there are supported with Go 1.7; it's Go 1.6 that does not have these values. As far as I can tell, you think you are building with Go 1.7, but you're actually building with Go 1.6.

Can you ensure that you aren't setting GOROOT? Is your build script setting a path to a different Go installation? (Several distributions have Go 1.6, so if your PATH points to your own built Go, but your build script has a different path, it could be using your OS Go installation.)

@richard-mauri
Copy link

@richard-mauri richard-mauri commented on a459706 Feb 16, 2017 via email •
edited by sethvargo
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richard-mauri
Copy link

@richard-mauri richard-mauri commented on a459706 Feb 16, 2017 via email •
edited by sethvargo
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jefferai
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richard-mauri You should probably look at vendoring, yes, if you need stable APIs.

Other than that there isn't any real way to protect/defend. I think that it's reasonable that versions of Go that have been abandoned by the Go team (e.g. for security updates) are considered to no longer be supported, and libraries move on to supported Go versions.

@glasser
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a somewhat more subtle note, this commit broke code that did

t := cleanhttp.DefaultTransport()
t.Dial = ...

because http.Transport uses DialContext preferentially over Dial.

Not saying this was a bad change to make, but just wanted to highlight that in case anyone else ran into the same issue.

Please sign in to comment.