Merge lp:~wallyworld/goose/better-http-transport-fix into lp:goose

[Ian Booth]

Do not overwrite http.Client's transport

Ensure that http.Client's transport has DisableKeepAlives=true
without ovwrwriting the transport itself.

https://codereview.appspot.com/14840043/

Do not overwrite http.Client's transport

Ensure that http.Client's transport has DisableKeepAlives=true
without ovwrwriting the transport itself.

https://codereview.appspot.com/14840043/

[Ian Booth (wallyworld)]

Reviewers: mp+191740_code.launchpad.net,

Message:
Please take a look.

Description:
Do not overwrite http.Client's transport

Ensure that http.Client's transport has DisableKeepAlives=true
without ovwrwriting the transport itself.

https://code.launchpad.net/~wallyworld/goose/better-http-transport-fix/+merge/191740

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14840043/

Affected files (+6, -3 lines):
   A [revision details]
   M http/client.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131017231035-89gpy8n7g2t928g8
+New revision: <email address hidden>

Index: http/client.go
=== modified file 'http/client.go'
--- http/client.go 2013-10-17 23:05:55 +0000
+++ http/client.go 2013-10-18 01:52:13 +0000
@@ -29,10 +29,11 @@
   // See https://code.google.com/p/go/issues/detail?id=4677
   // We need to force the connection to close each time so that we don't
   // hit the above Go bug.
- http.DefaultTransport = &http.Transport{
- Proxy: http.ProxyFromEnvironment,
- DisableKeepAlives: true,
+ roundTripper := http.DefaultClient.Transport
+ if transport, ok := roundTripper.(*http.Transport); ok {
+ transport.DisableKeepAlives = true
   }
+ http.DefaultTransport.(*http.Transport).DisableKeepAlives = true
  }

  type Client struct {

[Tim Penhey (thumper)]

https://codereview.appspot.com/14840043/diff/1/http/client.go
File http/client.go (right):

https://codereview.appspot.com/14840043/diff/1/http/client.go#newcode36
http/client.go:36:
http.DefaultTransport.(*http.Transport).DisableKeepAlives = true
You don't want this line right?

https://codereview.appspot.com/14840043/

[Ian Booth (wallyworld)]

https://codereview.appspot.com/14840043/diff/1/http/client.go
File http/client.go (right):

https://codereview.appspot.com/14840043/diff/1/http/client.go#newcode36
http/client.go:36:
http.DefaultTransport.(*http.Transport).DisableKeepAlives = true
On 2013/10/18 01:58:27, thumper wrote:
> You don't want this line right?

I think it's needed. What if a new http client is set up later and uses
the default transport. That's why I did this.

https://codereview.appspot.com/14840043/

[Tim Penhey (thumper)]

https://codereview.appspot.com/14840043/diff/1/http/client.go
File http/client.go (right):

https://codereview.appspot.com/14840043/diff/1/http/client.go#newcode36
http/client.go:36:
http.DefaultTransport.(*http.Transport).DisableKeepAlives = true
On 2013/10/18 02:02:54, wallyworld wrote:
> On 2013/10/18 01:58:27, thumper wrote:
> > You don't want this line right?

> I think it's needed. What if a new http client is set up later and
uses the
> default transport. That's why I did this.

Hmm.. ok. What about the other changes that are in gwacl, goose etc that
replaced the default transport?

https://codereview.appspot.com/14840043/

[Tim Penhey (thumper)]

On 2013/10/18 02:14:51, thumper wrote:
> https://codereview.appspot.com/14840043/diff/1/http/client.go
> File http/client.go (right):

https://codereview.appspot.com/14840043/diff/1/http/client.go#newcode36
> http/client.go:36:
http.DefaultTransport.(*http.Transport).DisableKeepAlives =
> true
> On 2013/10/18 02:02:54, wallyworld wrote:
> > On 2013/10/18 01:58:27, thumper wrote:
> > > You don't want this line right?
> >
> > I think it's needed. What if a new http client is set up later and
uses the
> > default transport. That's why I did this.

> Hmm.. ok. What about the other changes that are in gwacl, goose etc
that
> replaced the default transport?

Didn't notice that this one is goose.

LGTM

https://codereview.appspot.com/14840043/

=== modified file 'http/client.go'
--- http/client.go	2013-10-17 23:05:55 +0000
+++ http/client.go	2013-10-18 01:54:10 +0000
@@ -29,10 +29,11 @@
 	// See https://code.google.com/p/go/issues/detail?id=4677
 	// We need to force the connection to close each time so that we don't
 	// hit the above Go bug.
-	http.DefaultTransport = &http.Transport{
-		Proxy:             http.ProxyFromEnvironment,
-		DisableKeepAlives: true,
+	roundTripper := http.DefaultClient.Transport
+	if transport, ok := roundTripper.(*http.Transport); ok {
+		transport.DisableKeepAlives = true
 	}
+	http.DefaultTransport.(*http.Transport).DisableKeepAlives = true
 }
 
 type Client struct {