code review 68080044: runtime: deflake TestFinalizerType

runtime: deflake TestFinalizerType
I've run it 1e5 times and there seems to be flakes due to both
tiny alloc and concurrent sweep.
This change fixes all potential sources of flakyness.
Fixes issue 7328.

[dvyukov, 2014-02-24 14:05:18]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-02-24 16:47:23]

But what was the problem?



On Mon, Feb 24, 2014 at 6:05 AM, <dvyukov@google.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
>
>
> Description:
> runtime: deflake TestFinalizerType
> I've run it 1e5 times and there seems to be flakes due to both
> tiny alloc and concurrent sweep.
> This change fixes all potential sources of flakyness.
> Fixes issue 7328.
>
> Please review this at https://codereview.appspot.com/68080044/
>
> Affected files (+24, -19 lines):
>   M src/pkg/runtime/mfinal_test.go
>
>
> Index: src/pkg/runtime/mfinal_test.go
> ===================================================================
> --- a/src/pkg/runtime/mfinal_test.go
> +++ b/src/pkg/runtime/mfinal_test.go
> @@ -12,12 +12,12 @@
>         "time"
>  )
>
> -type Tintptr *int // assignable to *int
> -type Tint int     // *Tint implements Tinter, interface{}
> +type Tstrptr *string // assignable to *string
> +type Tstr string     // *Tstr implements Tstrer, interface{}
>
> -func (t *Tint) m() {}
> +func (t *Tstr) m() {}
>
> -type Tinter interface {
> +type Tstrer interface {
>         m()
>  }
>
> @@ -27,40 +27,45 @@
>         }
>
>         ch := make(chan bool, 10)
> -       finalize := func(x *int) {
> -               if *x != 97531 {
> -                       t.Errorf("finalizer %d, want %d", *x, 97531)
> +       finalize := func(x *string) {
> +               if *x != "97531" {
> +                       t.Errorf("finalizer %v, want %v", *x, "97531")
>                 }
>                 ch <- true
>         }
>
>         var finalizerTests = []struct {
> -               convert   func(*int) interface{}
> +               convert   func(*string) interface{}
>                 finalizer interface{}
>         }{
> -               {func(x *int) interface{} { return x }, func(v *int) {
> finalize(v) }},
> -               {func(x *int) interface{} { return Tintptr(x) }, func(v
> Tintptr) { finalize(v) }},
> -               {func(x *int) interface{} { return Tintptr(x) }, func(v
> *int) { finalize(v) }},
> -               {func(x *int) interface{} { return (*Tint)(x) }, func(v
> *Tint) { finalize((*int)(v)) }},
> -               {func(x *int) interface{} { return (*Tint)(x) }, func(v
> Tinter) { finalize((*int)(v.(*Tint))) }},
> +               {func(x *string) interface{} { return x }, func(v *string)
> { finalize(v) }},
> +               {func(x *string) interface{} { return Tstrptr(x) }, func(v
> Tstrptr) { finalize(v) }},
> +               {func(x *string) interface{} { return Tstrptr(x) }, func(v
> *string) { finalize(v) }},
> +               {func(x *string) interface{} { return (*Tstr)(x) }, func(v
> *Tstr) { finalize((*string)(v)) }},
> +               {func(x *string) interface{} { return (*Tstr)(x) }, func(v
> Tstrer) { finalize((*string)(v.(*Tstr))) }},
>         }
>
> +loop:
>         for _, tt := range finalizerTests {
>                 done := make(chan bool, 1)
>                 go func() {
> -                       v := new(int)
> -                       *v = 97531
> +                       v := new(string)
> +                       *v = "97531"
>                         runtime.SetFinalizer(tt.convert(v), tt.finalizer)
>                         v = nil
>                         done <- true
>                 }()
>                 <-done
>                 runtime.GC()
> -               select {
> -               case <-ch:
> -               case <-time.After(time.Second * 4):
> -                       t.Errorf("finalizer for type %T didn't run",
> tt.finalizer)
> +               for i := 0; i < 100; i++ {
> +                       select {
> +                       case <-ch:
> +                               continue loop
> +                       case <-time.After(100 * time.Millisecond):
> +                               runtime.GC()
> +                       }
>                 }
> +               t.Errorf("finalizer for type %T didn't run", tt.finalizer)
>         }
>  }
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[dvyukov, 2014-02-24 16:53:09]

I believe that one of the problems is new tiny allocator in runtime,
that sometimes prevents finalizer from running for objects < 16 bytes.
There can also be something related to concurrent sweep. Dubugging
such issues is basically impossible now.



On Mon, Feb 24, 2014 at 8:47 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> But what was the problem?
>
>
>
> On Mon, Feb 24, 2014 at 6:05 AM, <dvyukov@google.com> wrote:
>>
>> Reviewers: golang-codereviews,
>>
>> Message:
>> Hello golang-codereviews@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://dvyukov%40google.com@code.google.com/p/go/
>>
>>
>> Description:
>> runtime: deflake TestFinalizerType
>> I've run it 1e5 times and there seems to be flakes due to both
>> tiny alloc and concurrent sweep.
>> This change fixes all potential sources of flakyness.
>> Fixes issue 7328.
>>
>> Please review this at https://codereview.appspot.com/68080044/
>>
>> Affected files (+24, -19 lines):
>>   M src/pkg/runtime/mfinal_test.go
>>
>>
>> Index: src/pkg/runtime/mfinal_test.go
>> ===================================================================
>> --- a/src/pkg/runtime/mfinal_test.go
>> +++ b/src/pkg/runtime/mfinal_test.go
>> @@ -12,12 +12,12 @@
>>         "time"
>>  )
>>
>> -type Tintptr *int // assignable to *int
>> -type Tint int     // *Tint implements Tinter, interface{}
>> +type Tstrptr *string // assignable to *string
>> +type Tstr string     // *Tstr implements Tstrer, interface{}
>>
>> -func (t *Tint) m() {}
>> +func (t *Tstr) m() {}
>>
>> -type Tinter interface {
>> +type Tstrer interface {
>>         m()
>>  }
>>
>> @@ -27,40 +27,45 @@
>>         }
>>
>>         ch := make(chan bool, 10)
>> -       finalize := func(x *int) {
>> -               if *x != 97531 {
>> -                       t.Errorf("finalizer %d, want %d", *x, 97531)
>> +       finalize := func(x *string) {
>> +               if *x != "97531" {
>> +                       t.Errorf("finalizer %v, want %v", *x, "97531")
>>                 }
>>                 ch <- true
>>         }
>>
>>         var finalizerTests = []struct {
>> -               convert   func(*int) interface{}
>> +               convert   func(*string) interface{}
>>                 finalizer interface{}
>>         }{
>> -               {func(x *int) interface{} { return x }, func(v *int) {
>> finalize(v) }},
>> -               {func(x *int) interface{} { return Tintptr(x) }, func(v
>> Tintptr) { finalize(v) }},
>> -               {func(x *int) interface{} { return Tintptr(x) }, func(v
>> *int) { finalize(v) }},
>> -               {func(x *int) interface{} { return (*Tint)(x) }, func(v
>> *Tint) { finalize((*int)(v)) }},
>> -               {func(x *int) interface{} { return (*Tint)(x) }, func(v
>> Tinter) { finalize((*int)(v.(*Tint))) }},
>> +               {func(x *string) interface{} { return x }, func(v *string)
>> { finalize(v) }},
>> +               {func(x *string) interface{} { return Tstrptr(x) }, func(v
>> Tstrptr) { finalize(v) }},
>> +               {func(x *string) interface{} { return Tstrptr(x) }, func(v
>> *string) { finalize(v) }},
>> +               {func(x *string) interface{} { return (*Tstr)(x) }, func(v
>> *Tstr) { finalize((*string)(v)) }},
>> +               {func(x *string) interface{} { return (*Tstr)(x) }, func(v
>> Tstrer) { finalize((*string)(v.(*Tstr))) }},
>>         }
>>
>> +loop:
>>         for _, tt := range finalizerTests {
>>                 done := make(chan bool, 1)
>>                 go func() {
>> -                       v := new(int)
>> -                       *v = 97531
>> +                       v := new(string)
>> +                       *v = "97531"
>>                         runtime.SetFinalizer(tt.convert(v), tt.finalizer)
>>                         v = nil
>>                         done <- true
>>                 }()
>>                 <-done
>>                 runtime.GC()
>> -               select {
>> -               case <-ch:
>> -               case <-time.After(time.Second * 4):
>> -                       t.Errorf("finalizer for type %T didn't run",
>> tt.finalizer)
>> +               for i := 0; i < 100; i++ {
>> +                       select {
>> +                       case <-ch:
>> +                               continue loop
>> +                       case <-time.After(100 * time.Millisecond):
>> +                               runtime.GC()
>> +                       }
>>                 }
>> +               t.Errorf("finalizer for type %T didn't run", tt.finalizer)
>>         }
>>  }
>>
>>
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[dvyukov, 2014-02-26 03:51:35]

what exactly test do you mean?

On Tue, Feb 25, 2014 at 8:34 PM,  <nightlyone@googlemail.com> wrote:
> Does it make sense to add a Skipped test for this issue,
> so one can later know that it has been successfully fixed?
>
> Doing it that way is very common in 80/20 delivery situations like this.
>
> Would such a test be accepted?
>
> On Monday, February 24, 2014 5:52:48 PM UTC+1, Dmitry Vyukov wrote:
>> I believe that one of the problems is new tiny allocator in runtime,
>>
>> that sometimes prevents finalizer from running for objects < 16 bytes.
>>
>> There can also be something related to concurrent sweep. Dubugging
>>
>> such issues is basically impossible now.
>
> --
> You received this message because you are subscribed to the Google Groups
"golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.

[rsc, 2014-03-05 20:16:55]

R=close

i think the test is fixed