code review 61150043: net/http: close body in benchmarks

net/http: close body in benchmarks
Is it required? Why don't we do it?

[dvyukov, 2014-02-08 11:42:10]

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

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

[dvyukov, 2014-02-12 18:40:44]

On 2014/02/08 11:42:10, dvyukov wrote:
> Hello mailto:bradfitz@golang.org (cc:
mailto:golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/

ping

[dvyukov, 2014-02-12 18:40:45]

On 2014/02/08 11:42:10, dvyukov wrote:
> Hello mailto:bradfitz@golang.org (cc:
mailto:golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/

ping

[bradfitz, 2014-02-12 18:43:59]

https://codereview.appspot.com/61150043/diff/40001/src/pkg/net/http/serve_tes...
File src/pkg/net/http/serve_test.go (right):

https://codereview.appspot.com/61150043/diff/40001/src/pkg/net/http/serve_tes...
src/pkg/net/http/serve_test.go:2261: defer res.Body.Close()
don't defer. move down one line. don't need b.N defers.

https://codereview.appspot.com/61150043/diff/40001/src/pkg/net/http/serve_tes...
src/pkg/net/http/serve_test.go:2304: defer res.Body.Close()
don't defer it.  just put it after the ReadAll line.

https://codereview.appspot.com/61150043/diff/40001/src/pkg/net/http/serve_tes...
src/pkg/net/http/serve_test.go:2342: defer res.Body.Close()
don't defer. move down one line.

[dvyukov, 2014-02-16 08:27:30]

PTAL

https://codereview.appspot.com/61150043/diff/40001/src/pkg/net/http/serve_tes...
File src/pkg/net/http/serve_test.go (right):

https://codereview.appspot.com/61150043/diff/40001/src/pkg/net/http/serve_tes...
src/pkg/net/http/serve_test.go:2261: defer res.Body.Close()
On 2014/02/12 18:44:00, bradfitz wrote:
> don't defer. move down one line. don't need b.N defers.

Done.

https://codereview.appspot.com/61150043/diff/40001/src/pkg/net/http/serve_tes...
src/pkg/net/http/serve_test.go:2304: defer res.Body.Close()
On 2014/02/12 18:44:00, bradfitz wrote:
> don't defer it.  just put it after the ReadAll line.

Done.

https://codereview.appspot.com/61150043/diff/40001/src/pkg/net/http/serve_tes...
src/pkg/net/http/serve_test.go:2342: defer res.Body.Close()
On 2014/02/12 18:44:00, bradfitz wrote:
> don't defer. move down one line.

Done.

[bradfitz, 2014-02-16 17:04:47]

LGTM
 On Feb 16, 2014 12:27 AM, <dvyukov@google.com> wrote:

> PTAL
>
>
> https://codereview.appspot.com/61150043/diff/40001/src/
> pkg/net/http/serve_test.go
> File src/pkg/net/http/serve_test.go (right):
>
> https://codereview.appspot.com/61150043/diff/40001/src/
> pkg/net/http/serve_test.go#newcode2261
> src/pkg/net/http/serve_test.go:2261: defer res.Body.Close()
> On 2014/02/12 18:44:00, bradfitz wrote:
>
>> don't defer. move down one line. don't need b.N defers.
>>
>
> Done.
>
> https://codereview.appspot.com/61150043/diff/40001/src/
> pkg/net/http/serve_test.go#newcode2304
> src/pkg/net/http/serve_test.go:2304: defer res.Body.Close()
> On 2014/02/12 18:44:00, bradfitz wrote:
>
>> don't defer it.  just put it after the ReadAll line.
>>
>
> Done.
>
> https://codereview.appspot.com/61150043/diff/40001/src/
> pkg/net/http/serve_test.go#newcode2342
> src/pkg/net/http/serve_test.go:2342: defer res.Body.Close()
> On 2014/02/12 18:44:00, bradfitz wrote:
>
>> don't defer. move down one line.
>>
>
> Done.
>
> https://codereview.appspot.com/61150043/
>

[dvyukov, 2014-02-17 02:04:44]

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

net/http: close body in benchmarks
Is it required? Why don't we do it?

R=bradfitz
CC=golang-codereviews
https://codereview.appspot.com/61150043

[gobot, 2014-02-17 03:08:43]

This CL appears to have broken the windows-386-ec2 builder.

[fresh.grass.yum, 2014-02-17 12:22:50]

https://codereview.appspot.com/61150043/diff/80001/src/pkg/net/http/serve_tes...
File src/pkg/net/http/serve_test.go (right):

https://codereview.appspot.com/61150043/diff/80001/src/pkg/net/http/serve_tes...
src/pkg/net/http/serve_test.go:2261: defer res.Body.Close()
This line slipped through. It should have been removed, as per the comment from
bradfitz:
"src/pkg/net/http/serve_test.go:2261: defer res.Body.Close()
don't defer. move down one line. don't need b.N defers."

[dvyukov, 2014-02-17 17:47:55]

On Mon, Feb 17, 2014 at 4:22 PM,  <fresh.grass.yum@gmail.com> wrote:
>
>
https://codereview.appspot.com/61150043/diff/80001/src/pkg/net/http/serve_tes...
> File src/pkg/net/http/serve_test.go (right):
>
>
https://codereview.appspot.com/61150043/diff/80001/src/pkg/net/http/serve_tes...
> src/pkg/net/http/serve_test.go:2261: defer res.Body.Close()
> This line slipped through. It should have been removed, as per the
> comment from bradfitz:
> "src/pkg/net/http/serve_test.go:2261: defer res.Body.Close()
>
> don't defer. move down one line. don't need b.N defers."
>
> https://codereview.appspot.com/61150043/


Yes, thanks, there is already a CL mailed that fixes it.