code review 145930043: runtime: update docs, code for SetFinalizer

runtime: update docs, code for SetFinalizer

At last minute before 1.3 we relaxed SetFinalizer to avoid
crashes when you pass the result of a global alloc to it.
This avoids the crash but makes SetFinalizer a bit too relaxed.

Document that the finalizer of a global allocation may not run.

Tighten the SetFinalizer check to ignore a global allocation but
not ignore everything else.

Fixes issue 7656.

[rsc, 2014-10-06 15:27:20]

Hello golang-codereviews@googlegroups.com (cc: dvyukov, iant, khr, r, rlh),

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

[iant, 2014-10-06 16:00:14]

https://codereview.appspot.com/145930043/diff/40001/src/runtime/malloc.go
File src/runtime/malloc.go (right):

https://codereview.appspot.com/145930043/diff/40001/src/runtime/malloc.go#new...
src/runtime/malloc.go:573: }
It seems that we should have an error here.  Otherwise it's possible for a nil
base to get through the e.data != base test below and go on to set a finalizer
on nil.  For example this would happen on an attempt to set a finalizer on a
*C.int allocated in C code.

[rsc, 2014-10-06 16:13:46]

Hello golang-codereviews@googlegroups.com, iant@golang.org (cc:
dvyukov@google.com, golang-codereviews@googlegroups.com, khr@golang.org,
r@golang.org, rlh@golang.org),

Please take another look.

[rsc, 2014-10-06 16:14:37]

https://codereview.appspot.com/145930043/diff/40001/src/runtime/malloc.go
File src/runtime/malloc.go (right):

https://codereview.appspot.com/145930043/diff/40001/src/runtime/malloc.go#new...
src/runtime/malloc.go:573: }
On 2014/10/06 16:00:14, iant wrote:
> It seems that we should have an error here.  Otherwise it's possible for a nil
> base to get through the e.data != base test below and go on to set a finalizer
> on nil.  For example this would happen on an attempt to set a finalizer on a
> *C.int allocated in C code.

Done.

[r, 2014-10-06 16:25:28]

words LGTM

[iant, 2014-10-06 16:36:14]

LGTM

[rsc, 2014-10-06 18:18:15]

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

runtime: update docs, code for SetFinalizer

At last minute before 1.3 we relaxed SetFinalizer to avoid
crashes when you pass the result of a global alloc to it.
This avoids the crash but makes SetFinalizer a bit too relaxed.

Document that the finalizer of a global allocation may not run.

Tighten the SetFinalizer check to ignore a global allocation but
not ignore everything else.

Fixes issue 7656.

LGTM=r, iant
R=golang-codereviews, iant, r
CC=dvyukov, golang-codereviews, khr, rlh
https://codereview.appspot.com/145930043

[rlh, 2014-10-06 18:27:01]

LGTM
but with the caveat that we should not spec that finalizers will be run
promptly.

https://codereview.appspot.com/145930043/diff/60001/src/runtime/mfinal_test.go
File src/runtime/mfinal_test.go (right):

https://codereview.appspot.com/145930043/diff/60001/src/runtime/mfinal_test.g...
src/runtime/mfinal_test.go:68: t.Errorf("#%d: finalizer for type %T didn't run",
i, tt.finalizer)
This depends on the semantics of runtime.GC and an apparent assumptions that all
non-reachable objects will be discovered promptly by a runtime.GC and have their
finalizers promptly run. There are some corner cases where reachability provides
false positives for reasons other than conservative pointers. For example if a
stack frame holds a dead variables that point to otherwise unreachable object
but the compiler isn't bright enough to know they are dead variables then the
object will be considered reachable. While I suspect this will not be an issue I
wanted to mention it so that there is an awareness that the spec making
guarantees about the promptness of finalizers being run is not a good idea.

[rsc, 2014-10-06 18:34:11]

https://codereview.appspot.com/145930043/diff/60001/src/runtime/mfinal_test.go
File src/runtime/mfinal_test.go (right):

https://codereview.appspot.com/145930043/diff/60001/src/runtime/mfinal_test.g...
src/runtime/mfinal_test.go:68: t.Errorf("#%d: finalizer for type %T didn't run",
i, tt.finalizer)
On 2014/10/06 18:27:01, rlh wrote:
> This depends on the semantics of runtime.GC and an apparent assumptions that
all
> non-reachable objects will be discovered promptly by a runtime.GC and have
their
> finalizers promptly run. There are some corner cases where reachability
provides
> false positives for reasons other than conservative pointers. For example if a
> stack frame holds a dead variables that point to otherwise unreachable object
> but the compiler isn't bright enough to know they are dead variables then the
> object will be considered reachable. While I suspect this will not be an issue
I
> wanted to mention it so that there is an awareness that the spec making
> guarantees about the promptness of finalizers being run is not a good idea.

I don't believe the spec (doc comment for SetFinalizer) is making any such
guarantees. 

In the test, we do make this assumption, because we need some way to test
finalizer behavior so we have to do whatever it is that in the current
implementation would trigger the finalizers. But that's a testing detail, not a
statement of policy.

I do think that we will want runtime.GC to continue to mean 'do a full garbage
collection and reclaim everything you can and block until you've finished', even
if that gets more expensive. As far as I am concerned the only valid use of the
runtime.GC function is in tests like this one, and I think that's the best
meaning for a test.

[rlh, 2014-10-06 18:37:16]

LGTM

On Mon, Oct 6, 2014 at 2:34 PM, <rsc@golang.org> wrote:

>
> https://codereview.appspot.com/145930043/diff/60001/src/
> runtime/mfinal_test.go
> File src/runtime/mfinal_test.go (right):
>
> https://codereview.appspot.com/145930043/diff/60001/src/
> runtime/mfinal_test.go#newcode68
> src/runtime/mfinal_test.go:68: t.Errorf("#%d: finalizer for type %T
> didn't run", i, tt.finalizer)
> On 2014/10/06 18:27:01, rlh wrote:
>
>> This depends on the semantics of runtime.GC and an apparent
>>
> assumptions that all
>
>> non-reachable objects will be discovered promptly by a runtime.GC and
>>
> have their
>
>> finalizers promptly run. There are some corner cases where
>>
> reachability provides
>
>> false positives for reasons other than conservative pointers. For
>>
> example if a
>
>> stack frame holds a dead variables that point to otherwise unreachable
>>
> object
>
>> but the compiler isn't bright enough to know they are dead variables
>>
> then the
>
>> object will be considered reachable. While I suspect this will not be
>>
> an issue I
>
>> wanted to mention it so that there is an awareness that the spec
>>
> making
>
>> guarantees about the promptness of finalizers being run is not a good
>>
> idea.
>
> I don't believe the spec (doc comment for SetFinalizer) is making any
> such guarantees.
>
> In the test, we do make this assumption, because we need some way to
> test finalizer behavior so we have to do whatever it is that in the
> current implementation would trigger the finalizers. But that's a
> testing detail, not a statement of policy.
>
> I do think that we will want runtime.GC to continue to mean 'do a full
> garbage collection and reclaim everything you can and block until you've
> finished', even if that gets more expensive. As far as I am concerned
> the only valid use of the runtime.GC function is in tests like this one,
> and I think that's the best meaning for a test.
>
> https://codereview.appspot.com/145930043/
>

[gobot, 2014-10-07 02:29:45]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/7137fabc2ccb991ed0dad77f85959592aae8549c