code review 7777043: net/http: less flaky leaking goroutine test

net/http: less flaky leaking goroutine test

Fixes issue 5005

[bradfitz, 2013-03-12 23:59:50]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-03-13 00:01:59]

R=fullung for testing

Fixes issue 5005


On Tue, Mar 12, 2013 at 4:59 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: less flaky leaking goroutine test
>
> Please review this at
https://codereview.appspot.**com/7777043/<https://codereview.appspot.com/7777...
>
> Affected files:
>   M src/pkg/net/http/client_test.**go
>   M src/pkg/net/http/fs_test.go
>   M src/pkg/net/http/serve_test.go
>   M src/pkg/net/http/sniff_test.go
>   M src/pkg/net/http/transport_**test.go
>   M src/pkg/net/http/z_last_test.**go
>
>
>

[adg, 2013-03-13 00:07:05]

LGTM

Looks more reliable.

https://codereview.appspot.com/7777043/diff/5001/src/pkg/net/http/z_last_test.go
File src/pkg/net/http/z_last_test.go (right):

https://codereview.appspot.com/7777043/diff/5001/src/pkg/net/http/z_last_test...
src/pkg/net/http/z_last_test.go:49: func postTest(t *testing.T) {
postTest seems like it could be conflated with tests about POST requests

checkSideEffects ?

[bradfitz, 2013-03-13 00:33:20]

On Tue, Mar 12, 2013 at 5:07 PM, <adg@golang.org> wrote:

> LGTM
>
> Looks more reliable.
>

But it always was for me anyway, so I'll wait for fullung to beat on it.



> src/pkg/net/http/z_last_test.**go:49: func postTest(t *testing.T) {
> postTest seems like it could be conflated with tests about POST requests
>
> checkSideEffects ?
>

But it also does some clean-up (closing the default transport's idle
connections).  So it's some cleanup & some side effect checking.  It's like
"testCleanupAndCheckLeaks", but that's long.  Suggestions welcome.

[adg, 2013-03-13 00:42:43]

afterTest ?


On 13 March 2013 11:33, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> On Tue, Mar 12, 2013 at 5:07 PM, <adg@golang.org> wrote:
>
>> LGTM
>>
>> Looks more reliable.
>>
>
> But it always was for me anyway, so I'll wait for fullung to beat on it.
>
>
>
>> src/pkg/net/http/z_last_test.**go:49: func postTest(t *testing.T) {
>> postTest seems like it could be conflated with tests about POST requests
>>
>> checkSideEffects ?
>>
>
> But it also does some clean-up (closing the default transport's idle
> connections).  So it's some cleanup & some side effect checking.  It's like
> "testCleanupAndCheckLeaks", but that's long.  Suggestions welcome.
>
>

[albert.strasheim, 2013-03-14 06:25:00]

On 2013/03/13 00:01:59, bradfitz wrote:
> R=fullung for testing
> Fixes issue 5005

Still seeing some leaks.

Posted a complete log here:

https://code.google.com/p/go/issues/detail?id=5005#c16

[bradfitz, 2013-03-14 21:29:26]

Hello golang-dev@googlegroups.com, adg@golang.org, fullung@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[albert.strasheim, 2013-03-14 23:35:08]

getting closer I think

new log here:

https://code.google.com/p/go/issues/detail?id=5005#c18

[bradfitz, 2013-03-15 22:09:22]

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

net/http: less flaky leaking goroutine test

Fixes issue 5005

R=golang-dev, adg, fullung
CC=golang-dev
https://codereview.appspot.com/7777043

[bradfitz, 2013-03-15 22:10:01]

Submitted this for now, on adg's LGTM.

I can't reproduce any problems and I needed this out of my tree.  At least
now if there are failures, we won't see all the uninteresting goroutines.

On Fri, Mar 15, 2013 at 3:09 PM, <bradfitz@golang.org> wrote:

> *** Submitted as
>
https://code.google.com/p/go/**source/detail?r=871780d607f4<https://code.goog...
>
>
> net/http: less flaky leaking goroutine test
>
> Fixes issue 5005
>
> R=golang-dev, adg, fullung
> CC=golang-dev
>
https://codereview.appspot.**com/7777043<https://codereview.appspot.com/7777043>
>
>
>
https://codereview.appspot.**com/7777043/<https://codereview.appspot.com/7777...
>