code review 84850043: net/http: fix requests failing on short gzip body

net/http: fix requests failing on short gzip body

Fixes issue 7750.

[snaury, 2014-04-06 10:48:18]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[snaury, 2014-04-06 11:45:32]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[ibilicc, 2014-04-07 01:08:48]

On 2014/04/06 11:45:32, snaury wrote:
> Hello mailto:golang-codereviews@googlegroups.com (cc:
> mailto:golang-codereviews@googlegroups.com),
> 
> Please take another look.

This patch stopped the leaking of goroutines of my proxy checker
https://groups.google.com/forum/#!topic/golang-nuts/FnJZ9iZ0i_g

[bradfitz, 2014-04-09 18:27:42]

https://codereview.appspot.com/84850043/diff/60002/src/pkg/net/http/transport...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/84850043/diff/60002/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:913: t.Skip("skipping test; see
http://golang.org/issue/7237")
I think you just copy/pasted this from another test?

https://codereview.appspot.com/84850043/diff/60002/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:942: time.Sleep(400 * time.Millisecond)
can this test be done without the sleep & GC?  I imagine it can.  Exponential
backoff up to 400 milliseconds waiting for the number of goroutines to get back
to the starting state should do it.

If a sleep is actually required, it should probably both be higher, and the test
should be skipped by default.

This is a good bug report, but I might fix it slightly differently.  I have
another fix coming which might end up fixing this one as well, but this is a
good test addition in general.

[snaury, 2014-04-09 21:13:26]

Please take another look.

https://codereview.appspot.com/84850043/diff/60002/src/pkg/net/http/transport...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/84850043/diff/60002/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:913: t.Skip("skipping test; see
http://golang.org/issue/7237")
On 2014/04/09 18:27:43, bradfitz wrote:
> I think you just copy/pasted this from another test?

Yes, since other similar tests have this skip I copied it just in case.

https://codereview.appspot.com/84850043/diff/60002/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:942: time.Sleep(400 * time.Millisecond)
On 2014/04/09 18:27:43, bradfitz wrote:
> can this test be done without the sleep & GC?  I imagine it can.  Exponential
> backoff up to 400 milliseconds waiting for the number of goroutines to get
back
> to the starting state should do it.

I didn't do exponential backoff or get rid of gc, but I did two things:

- loop with 20 millisecond sleeps
- stop the test early if there's no error returned from the client

Since my patch returns a valid response (as it should) there's nothing to test
for. But it is now designed to actually fail with the original code.

Also, why not use GC? Other tests do.

[snaury, 2014-04-10 05:44:58]

Since CL 86290043 appears to fix the leak I'm repurposing this CL for a
different issue.

Please take another look.

[snaury, 2014-04-10 05:46:37]

Hello golang-codereviews@googlegroups.com, ibilicc@gmail.com,
bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-04-10 17:31:40]

Looks pretty good. Few little comments.

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
src/pkg/net/http/transport.go:1157: reader io.Reader
call this "zr" instead of reader.

   type gzipReader struct {
         body io.ReadCloser // underlying Response.Body
         zr   io.Reader // lazily-initialized gzip reader
   }

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
src/pkg/net/http/transport_test.go:816: c := &Client{Transport: &Transport{}}
you'll want to put &Transport{} in its own variable, and then call defer
tr.CloseIdleConnections():

  tr := &Transport{}
  defer tr.CloseIdleConnections()
  c := &Client{Transport: tr}

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
src/pkg/net/http/transport_test.go:821: _, err = ioutil.ReadAll(res.Body)
before this line:

   defer res.Body.Close()

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
src/pkg/net/http/transport_test.go:826: t.Fatal(err)
t.Errorf("ReadAll error = %v; want io.ErrUnexpectedEOF", err)

[snaury, 2014-04-10 20:11:21]

Please take another look.

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
src/pkg/net/http/transport.go:1157: reader io.Reader
On 2014/04/10 17:31:40, bradfitz wrote:
> call this "zr" instead of reader.
> 
>    type gzipReader struct {
>          body io.ReadCloser // underlying Response.Body
>          zr   io.Reader // lazily-initialized gzip reader
>    }
> 

Done.

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
src/pkg/net/http/transport_test.go:816: c := &Client{Transport: &Transport{}}
On 2014/04/10 17:31:40, bradfitz wrote:
> you'll want to put &Transport{} in its own variable, and then call defer
> tr.CloseIdleConnections():
> 
>   tr := &Transport{}
>   defer tr.CloseIdleConnections()
>   c := &Client{Transport: tr}

Done.

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
src/pkg/net/http/transport_test.go:821: _, err = ioutil.ReadAll(res.Body)
On 2014/04/10 17:31:40, bradfitz wrote:
> before this line:
> 
>    defer res.Body.Close()

Done.

https://codereview.appspot.com/84850043/diff/150001/src/pkg/net/http/transpor...
src/pkg/net/http/transport_test.go:826: t.Fatal(err)
On 2014/04/10 17:31:40, bradfitz wrote:
> t.Errorf("ReadAll error = %v; want io.ErrUnexpectedEOF", err)

Done.

[bradfitz, 2014-04-10 21:11:47]

LGTM

[bradfitz, 2014-04-10 21:12:39]

*** Submitted as https://code.google.com/p/go/source/detail?r=60c7a236d3bc ***

net/http: fix requests failing on short gzip body

Fixes issue 7750.

LGTM=bradfitz
R=golang-codereviews, ibilicc, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/84850043

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[gobot, 2014-04-10 21:32:45]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/c40a090ca04c96f2dd63f7e5782cf1a32f6b0d98