code review 6842109: net/http: fix broken TestIssue4191_InfiniteGetToPutTimeout

net/http: fix broken TestIssue4191_InfiniteGetToPutTimeout

Test creates 2 tcp connections for put and get. Make sure
these are closed properly after test is over, otherwise
server hangs waiting for connection to be closed.

[brainman, 2012-11-28 04:19:06]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-11-28 04:30:23]

https://codereview.appspot.com/6842109/diff/3002/src/pkg/net/http/transport_t...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/6842109/diff/3002/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:988: defer sres.Body.Close()
I thought the problem was Issue 4434, but I guess I could see this being a
problem too.

This one should time out anyway, since the net.Conn came from the the same
dialer which times things out.  But maybe the PUT write fails first, and then
this is just never read enough to see timeout error (which would also close
things).

So yeah, this is fine, but:

https://codereview.appspot.com/6842109/diff/3002/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:994: defer req.Body.Close()
this line isn't necessary.  you don't need to close the bodies of requests, and
besides: you already closed the same ReadCloser on line 988.  NewRequest creates
req from sres.Body.

[brainman, 2012-11-28 04:52:49]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2012-11-28 04:52:51]

https://codereview.appspot.com/6842109/diff/3002/src/pkg/net/http/transport_t...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/6842109/diff/3002/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:988: defer sres.Body.Close()
On 2012/11/28 04:30:23, bradfitz wrote:
> ...  But maybe the PUT write fails first, and then
> this is just never read enough to see timeout error ...

Yes, that is what I see here.

https://codereview.appspot.com/6842109/diff/3002/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:994: defer req.Body.Close()
On 2012/11/28 04:30:23, bradfitz wrote:
> this line isn't necessary. ...

I take your word for it. Removed.

[bradfitz, 2012-11-28 05:37:48]

LGTM

Thanks.

[dave_cheney.net, 2012-11-28 05:48:28]

LGTM. Passed 200 cycles.

https://codereview.appspot.com/6842109/diff/3003/src/pkg/net/http/transport_t...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/6842109/diff/3003/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:989: println("run", i+1, "of", nRuns)
Please make this t.Logf(), and remove the debug condition

https://codereview.appspot.com/6842109/diff/3003/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:993: t.Errorf("Error issuing GET: %v", err)
Do you need sres.Body.Close() here as well ?

[bradfitz, 2012-11-28 05:49:53]

https://codereview.appspot.com/6842109/diff/3003/src/pkg/net/http/transport_t...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/6842109/diff/3003/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:989: println("run", i+1, "of", nRuns)
On 2012/11/28 05:48:28, dfc wrote:
> Please make this t.Logf(), and remove the debug condition

Please not yet.  I want to make t.Logf unbuffered first when only one test is
running not in parallel, in the testing package.

Without println I can't actually see real-time output and it's super annoying.

https://codereview.appspot.com/6842109/diff/3003/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:993: t.Errorf("Error issuing GET: %v", err)
On 2012/11/28 05:48:28, dfc wrote:
> Do you need sres.Body.Close() here as well ?

BOOM. sres is nil.

[dave_cheney.net, 2012-11-28 05:52:08]

https://codereview.appspot.com/6842109/diff/3003/src/pkg/net/http/transport_t...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/6842109/diff/3003/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:989: println("run", i+1, "of", nRuns)
On 2012/11/28 05:49:53, bradfitz wrote:
> On 2012/11/28 05:48:28, dfc wrote:
> > Please make this t.Logf(), and remove the debug condition
> 
> Please not yet.  I want to make t.Logf unbuffered first when only one test is
> running not in parallel, in the testing package.
> 
> Without println I can't actually see real-time output and it's super annoying.

Ok, makes sense.

https://codereview.appspot.com/6842109/diff/3003/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:993: t.Errorf("Error issuing GET: %v", err)
On 2012/11/28 05:49:53, bradfitz wrote:
> On 2012/11/28 05:48:28, dfc wrote:
> > Do you need sres.Body.Close() here as well ?
> 
> BOOM. sres is nil.

Sorry, you are correct, I was overreacting.

[brainman, 2012-11-28 05:56:30]

So, am I submitting as is?

Alex

[dave_cheney.net, 2012-11-28 05:59:51]

Yes please.

On Wed, Nov 28, 2012 at 4:56 PM,  <alex.brainman@gmail.com> wrote:
> So, am I submitting as is?
>
> Alex
>
> https://codereview.appspot.com/6842109/

[brainman, 2012-11-28 06:01:00]

*** Submitted as http://code.google.com/p/go/source/detail?r=e76e1b1e9d4d ***

net/http: fix broken TestIssue4191_InfiniteGetToPutTimeout

Test creates 2 tcp connections for put and get. Make sure
these are closed properly after test is over, otherwise
server hangs waiting for connection to be closed.

R=golang-dev, bradfitz, dave
CC=golang-dev
http://codereview.appspot.com/6842109