code review 11858043: runtime: allow SetFinalizer with a func(interface{})

runtime: allow SetFinalizer with a func(interface{})

Fixes issue 5368.

[peted, 2013-07-25 19:47:08]

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

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

[dvyukov, 2013-07-26 09:55:04]

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:851: uintptr nret = 0;
do not initialize variable during declaration

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:870: if (ft->dotdotdot || ft->in.len != 1)
remove the space

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:873: if (fint->kind == KindInterface &&
((InterfaceType*)fint)->mhdr.len == 0) {
no space between "if("

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:873: if (fint->kind == KindInterface &&
((InterfaceType*)fint)->mhdr.len == 0) {
drop unnecessary {}

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:875: } else if (fint != obj.type) {
no space between "if("

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:875: } else if (fint != obj.type) {
drop unnecessary {}

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test.go
File src/pkg/runtime/mfinal_test.go (right):

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:15: func TestFinalizerTypeSucceed(t *testing.T) {
run these tests iff runtime.GOARCH=amd64, 32-bit platforms has problems with GC
precision and may not run any finalizers at all

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:16: ticker := time.After(time.Second * 4)
just write:
case <-time.After(...):
below

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:18: f := func(v *int) {
declare it inline in SetFinalizer call

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:20: if *v != 97531 {
you need to check before chan close, otherwise the test may have already been
finished

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:29: // otherwise this function gets optimized
out.
I do't think this is the case. gc is not all that agressive and this function
does have side effects.
probably it just leaves some pointers on stack

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:2302: framesz = argsz + f->nret;
framesz = sizeof(Eface) + f->nret;
The additional word is absolutely not important here

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:2312: if (f->ot == nil) {
drop {}

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:2315: Eface *ef = (Eface*)frame;
move the declaration to the top of the function

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/runtime.h#n...
src/pkg/runtime/runtime.h:811: bool	runtime·addfinalizer(void*, FuncVal *fn,
uintptr, void *);
no space between "void *" to match the code around

[peted, 2013-07-26 23:31:24]

All done except src/pkg/runtime/mfinal_test.go:29.

What do you propose?

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:851: uintptr nret = 0;
On 2013/07/26 09:55:04, dvyukov wrote:
> do not initialize variable during declaration

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:870: if (ft->dotdotdot || ft->in.len != 1)
On 2013/07/26 09:55:04, dvyukov wrote:
> remove the space

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:873: if (fint->kind == KindInterface &&
((InterfaceType*)fint)->mhdr.len == 0) {
On 2013/07/26 09:55:04, dvyukov wrote:
> no space between "if("

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:873: if (fint->kind == KindInterface &&
((InterfaceType*)fint)->mhdr.len == 0) {
On 2013/07/26 09:55:04, dvyukov wrote:
> drop unnecessary {}

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:875: } else if (fint != obj.type) {
On 2013/07/26 09:55:04, dvyukov wrote:
> no space between "if("

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:875: } else if (fint != obj.type) {
On 2013/07/26 09:55:04, dvyukov wrote:
> no space between "if("

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test.go
File src/pkg/runtime/mfinal_test.go (right):

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:15: func TestFinalizerTypeSucceed(t *testing.T) {
On 2013/07/26 09:55:04, dvyukov wrote:
> run these tests iff runtime.GOARCH=amd64, 32-bit platforms has problems with
GC
> precision and may not run any finalizers at all

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:16: ticker := time.After(time.Second * 4)
On 2013/07/26 09:55:04, dvyukov wrote:
> just write:
> case <-time.After(...):
> below

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:18: f := func(v *int) {
On 2013/07/26 09:55:04, dvyukov wrote:
> declare it inline in SetFinalizer call

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:20: if *v != 97531 {
On 2013/07/26 09:55:04, dvyukov wrote:
> you need to check before chan close, otherwise the test may have already been
> finished

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:29: // otherwise this function gets optimized
out.
Perhaps. Fact remains that removing this makes it fail. What do you propose?

On 2013/07/26 09:55:04, dvyukov wrote:
> I do't think this is the case. gc is not all that agressive and this function
> does have side effects.
> probably it just leaves some pointers on stack

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:2302: framesz = argsz + f->nret;
On 2013/07/26 09:55:04, dvyukov wrote:
> framesz = sizeof(Eface) + f->nret;
> The additional word is absolutely not important here

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:2312: if (f->ot == nil) {
On 2013/07/26 09:55:04, dvyukov wrote:
> drop {}

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:2315: Eface *ef = (Eface*)frame;
On 2013/07/26 09:55:04, dvyukov wrote:
> move the declaration to the top of the function

Done.

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/runtime.h#n...
src/pkg/runtime/runtime.h:811: bool	runtime·addfinalizer(void*, FuncVal *fn,
uintptr, void *);
On 2013/07/26 09:55:04, dvyukov wrote:
> no space between "void *" to match the code around

Done.

[peted, 2013-07-26 23:33:40]

Hello golang-dev@googlegroups.com, dvyukov@google.com (cc:
golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[dvyukov, 2013-07-27 10:06:35]

On Sat, Jul 27, 2013 at 3:31 AM,  <pieter@binky.org.uk> wrote:
> All done except src/pkg/runtime/mfinal_test.go:29.
>
> What do you propose?


The test works for me on darwin/amd64 with t.Skipped() commented out.
Does it fail for you now?


https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc
> File src/pkg/runtime/malloc.goc (right):
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
> src/pkg/runtime/malloc.goc:851: uintptr nret = 0;
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> do not initialize variable during declaration
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
> src/pkg/runtime/malloc.goc:870: if (ft->dotdotdot || ft->in.len != 1)
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> remove the space
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
> src/pkg/runtime/malloc.goc:873: if (fint->kind == KindInterface &&
> ((InterfaceType*)fint)->mhdr.len == 0) {
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> no space between "if("
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
> src/pkg/runtime/malloc.goc:873: if (fint->kind == KindInterface &&
> ((InterfaceType*)fint)->mhdr.len == 0) {
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> drop unnecessary {}
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
> src/pkg/runtime/malloc.goc:875: } else if (fint != obj.type) {
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> no space between "if("
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/malloc.goc#...
> src/pkg/runtime/malloc.goc:875: } else if (fint != obj.type) {
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> no space between "if("
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test.go
> File src/pkg/runtime/mfinal_test.go (right):
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
> src/pkg/runtime/mfinal_test.go:15: func TestFinalizerTypeSucceed(t
> *testing.T) {
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> run these tests iff runtime.GOARCH=amd64, 32-bit platforms has
>
> problems with GC
>>
>> precision and may not run any finalizers at all
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
> src/pkg/runtime/mfinal_test.go:16: ticker := time.After(time.Second * 4)
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> just write:
>> case <-time.After(...):
>> below
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
> src/pkg/runtime/mfinal_test.go:18: f := func(v *int) {
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> declare it inline in SetFinalizer call
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
> src/pkg/runtime/mfinal_test.go:20: if *v != 97531 {
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> you need to check before chan close, otherwise the test may have
>
> already been
>>
>> finished
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mfinal_test...
> src/pkg/runtime/mfinal_test.go:29: // otherwise this function gets
> optimized out.
> Perhaps. Fact remains that removing this makes it fail. What do you
> propose?
>
>
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> I do't think this is the case. gc is not all that agressive and this
>
> function
>>
>> does have side effects.
>> probably it just leaves some pointers on stack
>
>
> https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c
> File src/pkg/runtime/mgc0.c (right):
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c#newc...
> src/pkg/runtime/mgc0.c:2302: framesz = argsz + f->nret;
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> framesz = sizeof(Eface) + f->nret;
>> The additional word is absolutely not important here
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c#newc...
> src/pkg/runtime/mgc0.c:2312: if (f->ot == nil) {
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> drop {}
>
>
> Done.
>
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/mgc0.c#newc...
> src/pkg/runtime/mgc0.c:2315: Eface *ef = (Eface*)frame;
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> move the declaration to the top of the function
>
>
> Done.
>
>
> https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/runtime.h
> File src/pkg/runtime/runtime.h (right):
>
>
https://codereview.appspot.com/11858043/diff/5001/src/pkg/runtime/runtime.h#n...
> src/pkg/runtime/runtime.h:811: bool     runtime·addfinalizer(void*, FuncVal
> *fn, uintptr, void *);
> On 2013/07/26 09:55:04, dvyukov wrote:
>>
>> no space between "void *" to match the code around
>
>
> Done.
>
> https://codereview.appspot.com/11858043/

[peted, 2013-07-27 10:16:48]

On 2013/07/27 10:06:35, dvyukov wrote:
> On Sat, Jul 27, 2013 at 3:31 AM,  <mailto:pieter@binky.org.uk> wrote:
> > All done except src/pkg/runtime/mfinal_test.go:29.
> >
> > What do you propose?
> 
> 
> The test works for me on darwin/amd64 with t.Skipped() commented out.
> Does it fail for you now?

Indeed, it does work now. Magic.

Anything else?

[dvyukov, 2013-07-27 10:18:52]

On 2013/07/27 10:16:48, peted wrote:
> On 2013/07/27 10:06:35, dvyukov wrote:
> > On Sat, Jul 27, 2013 at 3:31 AM,  <mailto:pieter@binky.org.uk> wrote:
> > > All done except src/pkg/runtime/mfinal_test.go:29.
> > >
> > > What do you propose?
> > 
> > 
> > The test works for me on darwin/amd64 with t.Skipped() commented out.
> > Does it fail for you now?
> 
> Indeed, it does work now. Magic.
> 
> Anything else?

LGTM if you remove t.Skipped()

[peted, 2013-07-27 10:20:34]

Hello golang-dev@googlegroups.com, dvyukov@google.com (cc:
golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[dvyukov, 2013-07-29 15:43:51]

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

runtime: allow SetFinalizer with a func(interface{})

Fixes issue 5368.

R=golang-dev, dvyukov
CC=golang-dev, rsc
https://codereview.appspot.com/11858043

Committer: Dmitriy Vyukov <dvyukov@google.com>

[rsc, 2013-07-29 17:16:01]

I would have liked to have been given a chance to look at this before it was
submitted.

Why is interface{} special? Why do we not allow any argument type to which x can
be assigned?

Why are there only tests on amd64?

[dvyukov, 2013-07-29 17:29:49]

On Mon, Jul 29, 2013 at 9:16 PM,  <rsc@golang.org> wrote:
> I would have liked to have been given a chance to look at this before it
> was submitted.
>
> Why is interface{} special? Why do we not allow any argument type to
> which x can be assigned?

There are 2 use cases:
1. Concrete finalizer for concrete type -- this is where current
finalizer is used.
2. Generic reflect-based programming -- this is where interface{}
finalizer is necessary.

> Why are there only tests on amd64?

Finalizer tests are flaky on 32-bits because of conservative GC. We
use such check in test/ for finalizer tests as well.

[peted, 2013-07-29 20:12:03]

On 2013/07/29 17:29:49, dvyukov wrote:
> On Mon, Jul 29, 2013 at 9:16 PM,  <mailto:rsc@golang.org> wrote:
> > I would have liked to have been given a chance to look at this before it
> > was submitted.

If my impatience caused trouble, I apologise.

> > Why is interface{} special? Why do we not allow any argument type to
> > which x can be assigned?
> 
> There are 2 use cases:
> 1. Concrete finalizer for concrete type -- this is where current
> finalizer is used.
> 2. Generic reflect-based programming -- this is where interface{}
> finalizer is necessary.

Well, it would be nice if SetFinalizer() accepted any f whose argument is an
interface implemented by x. I can imagine this finalizer quite easily:

func fin(x io.Closer) {
    x.Close()
}

Not exactly idiomatic, but not unthinkable. Cleaner and more descriptive than
interface{}, and interface{} wouldn't feel like such a special case.

Some more stuff would have to be moved to the runtime package. The body of
reflect.implements() comes to mind, unless you want to go further than that
(AssignableTo? ConvertibleTo?). And some more digging around the type system.

But it does sound like a seperate issue from allowing finalizers to be used with
reflect.

[rsc, 2013-07-30 23:28:27]

I'll think about it and figure out whether I want to change anything.
Thanks.

[rsc, 2013-08-13 21:30:45]

CL https://codereview.appspot.com/12895043 expands this.

[fresh.grass.yum, 2013-09-19 23:18:40]

https://codereview.appspot.com/11858043/diff/31001/src/pkg/runtime/mfinal_tes...
File src/pkg/runtime/mfinal_test.go (right):

https://codereview.appspot.com/11858043/diff/31001/src/pkg/runtime/mfinal_tes...
src/pkg/runtime/mfinal_test.go:83: t.Errorf("Expected *bigValue from interface{}
in finalizer, got %v", *i)
If I read this correctly the type "*bigValue" was expected. If that is the case
the printf verb "%v" should probably be replaced with "%T" and "*i" with "i"?

t.Errorf("Expected *bigValue from interface{} in finalizer, got %T", i)

https://codereview.appspot.com/11858043/diff/31001/src/pkg/runtime/mfinal_tes...
src/pkg/runtime/mfinal_test.go:85: if i.fill != 0xDEADBEEFDEADBEEF && i.it !=
true && i.up != "It matters not how strait the gate" {
Currently the test will only fail if all values are incorrect. What if one is
incorrect? Either "||" should be used instead of "&&" but I would rather want to
split the test into three different ones to give better error reporting.

Currently "%d" is an invalid printf verb for "*i".

What is the expected output if this fails?

Suggested replacement:
if i.fill != 0xDEADBEEFDEADBEEF {
	t.Errorf("Expected %d, got %d.", 0xDEADBEEFDEADBEEF, i.fill)
}
if i.it != true {
	t.Errorf("Expected %t, got %t.", true, i.it)
}
if i.up != "It matters not how strait the gate" {
	t.Errorf("Expected %q, got %q.", "It matters not how strait the gate", i.up)
}

If these tests are rewritten in the future it would be nice to make them table
driven instead, this would reduce duplicate entries of strings such as: "It
matters not how strait the gate" (which can lead to typos).