code review 8954044: runtime: clear the stack frame used in runfinq()

runtime: prevent the GC from seeing the content of a frame in runfinq()

Fixes issue 5348.

[atom, 2013-04-25 09:34:08]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-04-25 09:59:58]

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

https://codereview.appspot.com/8954044/diff/5001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:2206: runtime·memclr(frame, framecap);
Just allocate the frame with FlagNoPointers:
runtime.mallocgc(framesz, FlagNoPointers, 1, 0)

With your solution frame will prevent potential return values from being GCed
while the frame is not zeroed. And also unnecessary calls to memclr and scanning
by GC.

https://codereview.appspot.com/8954044/diff/5001/test/fixedbugs/issue5348.go
File test/fixedbugs/issue5348.go (right):

https://codereview.appspot.com/8954044/diff/5001/test/fixedbugs/issue5348.go#...
test/fixedbugs/issue5348.go:33: runtime.GC()
I do not think that we want to retain runtime.gc() call in runfinq(). Because if
you have 10GB heap and a single finalizer, you pay double price with no
particular benefit. Of even worse if you have a linked list with finalizers.

Please add the second GC() call to be more explicit.

[atom, 2013-04-25 10:52:19]

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

https://codereview.appspot.com/8954044/diff/5001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:2206: runtime·memclr(frame, framecap);
On 2013/04/25 09:59:58, dvyukov wrote:
> Just allocate the frame with FlagNoPointers:
> runtime.mallocgc(framesz, FlagNoPointers, 1, 0)
> 
> With your solution frame will prevent potential return values from being GCed
> while the frame is not zeroed. And also unnecessary calls to memclr and
scanning
> by GC.

Done.

https://codereview.appspot.com/8954044/diff/5001/test/fixedbugs/issue5348.go
File test/fixedbugs/issue5348.go (right):

https://codereview.appspot.com/8954044/diff/5001/test/fixedbugs/issue5348.go#...
test/fixedbugs/issue5348.go:33: runtime.GC()
On 2013/04/25 09:59:58, dvyukov wrote:
> I do not think that we want to retain runtime.gc() call in runfinq(). Because
if
> you have 10GB heap and a single finalizer, you pay double price with no
> particular benefit. Of even worse if you have a linked list with finalizers.
> 
> Please add the second GC() call to be more explicit.

Done.

[dvyukov, 2013-04-25 10:55:21]

LGTM

[atom, 2013-04-25 11:39:21]

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

runtime: prevent the GC from seeing the content of a frame in runfinq()

Fixes issue 5348.

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

[dave_cheney.net, 2013-04-25 12:31:00]

This commit has caused several build breakages, please undo it.

On Thu, Apr 25, 2013 at 9:39 PM,  <0xE2.0x9A.0x9B@gmail.com> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=ad3c2ffb16d7 ***
>
> runtime: prevent the GC from seeing the content of a frame in runfinq()
>
> Fixes issue 5348.
>
> R=golang-dev, dvyukov
> CC=golang-dev
> https://codereview.appspot.com/8954044
>
>
>
> https://codereview.appspot.com/8954044/
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[atom, 2013-04-25 12:57:24]

On 2013/04/25 12:31:00, dfc wrote:
> This commit has caused several build breakages, please undo it.

I am investigating the cause on amd64. Please wait.

> On Thu, Apr 25, 2013 at 9:39 PM,  <mailto:0xE2.0x9A.0x9B@gmail.com> wrote:
> > *** Submitted as
> > https://code.google.com/p/go/source/detail?r=ad3c2ffb16d7 ***
> >
> > runtime: prevent the GC from seeing the content of a frame in runfinq()
> >
> > Fixes issue 5348.
> >
> > R=golang-dev, dvyukov
> > CC=golang-dev
> > https://codereview.appspot.com/8954044
> >
> >
> >
> > https://codereview.appspot.com/8954044/
> >
> > --
> >
> > ---You received this message because you are subscribed to the Google Groups
> > "golang-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:golang-dev+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.
> >
> >

[iant, 2013-04-25 14:25:31]

On Thu, Apr 25, 2013 at 5:57 AM,  <0xE2.0x9A.0x9B@gmail.com> wrote:
> On 2013/04/25 12:31:00, dfc wrote:
>>
>> This commit has caused several build breakages, please undo it.

Also as we are in the run-up to the 1.1 release, please only commit
changes after explicit approval from r or adg.  Thanks.

Ian

> I am investigating the cause on amd64. Please wait.
>
>> On Thu, Apr 25, 2013 at 9:39 PM,  <mailto:0xE2.0x9A.0x9B@gmail.com>
>
> wrote:
>>
>> > *** Submitted as
>> > https://code.google.com/p/go/source/detail?r=ad3c2ffb16d7 ***
>> >
>> > runtime: prevent the GC from seeing the content of a frame in
>
> runfinq()
>>
>> >
>> > Fixes issue 5348.
>> >
>> > R=golang-dev, dvyukov
>> > CC=golang-dev
>> > https://codereview.appspot.com/8954044
>> >
>> >
>> >
>> > https://codereview.appspot.com/8954044/
>> >
>> > --
>> >
>> > ---You received this message because you are subscribed to the
>
> Google Groups
>>
>> > "golang-dev" group.
>> > To unsubscribe from this group and stop receiving emails from it,
>
> send an
>>
>> > email to mailto:golang-dev+unsubscribe@googlegroups.com.
>>
>> > For more options, visit https://groups.google.com/groups/opt_out.
>> >
>> >
>
>
>
>
> https://codereview.appspot.com/8954044/
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[atom, 2013-04-25 14:47:13]

GC on amd64 found (in a stack frame) an integer value matching the
to-be-finalized pointer.

I would suggest to skip test/fixedbugs/issue5348.go and let it begin with the
following two lines:

// skip
// Replace "skip" with "run" when the garbage collector is precise enough.

This means submitting a new CL containing the above two lines, instead of
undoing this CL. Alternatively, the new CL could delete file issue5348.go.

Waiting for r to make the final decision.

On 2013/04/25 14:25:31, iant wrote:
> On Thu, Apr 25, 2013 at 5:57 AM,  <mailto:0xE2.0x9A.0x9B@gmail.com> wrote:
> > On 2013/04/25 12:31:00, dfc wrote:
> >>
> >> This commit has caused several build breakages, please undo it.
> 
> Also as we are in the run-up to the 1.1 release, please only commit
> changes after explicit approval from r or adg.  Thanks.
> 
> Ian
> 
> > I am investigating the cause on amd64. Please wait.
> >
> >> On Thu, Apr 25, 2013 at 9:39 PM,  <mailto:0xE2.0x9A.0x9B@gmail.com>
> >
> > wrote:
> >>
> >> > *** Submitted as
> >> > https://code.google.com/p/go/source/detail?r=ad3c2ffb16d7 ***
> >> >
> >> > runtime: prevent the GC from seeing the content of a frame in
> >
> > runfinq()
> >>
> >> >
> >> > Fixes issue 5348.
> >> >
> >> > R=golang-dev, dvyukov
> >> > CC=golang-dev
> >> > https://codereview.appspot.com/8954044
> >> >
> >> >
> >> >
> >> > https://codereview.appspot.com/8954044/
> >> >
> >> > --
> >> >
> >> > ---You received this message because you are subscribed to the
> >
> > Google Groups
> >>
> >> > "golang-dev" group.
> >> > To unsubscribe from this group and stop receiving emails from it,
> >
> > send an
> >>
> >> > email to mailto:golang-dev+unsubscribe@googlegroups.com.
> >>
> >> > For more options, visit https://groups.google.com/groups/opt_out.
> >> >
> >> >
> >
> >
> >
> >
> > https://codereview.appspot.com/8954044/
> >
> > --
> >
> > ---You received this message because you are subscribed to the Google Groups
> > "golang-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:golang-dev+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.
> >
> >

[r, 2013-04-25 14:59:44]

Neither adg nor I had signed off on this CL yet. In the runup to the release,
please don't check things in without the explicit approval of one of us.

[r, 2013-04-25 15:04:29]

Is your suggest that we roll back a fix that has caught a bug in the runtime?

I would prefer you roll back this change, which just replaces one bug
with another. It's not clear to me that the original bug is a
show-stopper.

-rob

[r, 2013-04-25 15:05:33]

I didn't say that well: You are asking whether we should disable a
test that catches a bug in the runtime. I suggest instead we roll back
this whole change until we understand better what is going on.

-rob

[0xe2.0x9a.0x9b_gmail.com, 2013-04-25 15:10:13]

On Thu, Apr 25, 2013 at 5:05 PM, Rob Pike <r@golang.org> wrote:

> I didn't say that well: You are asking whether we should disable a
> test that catches a bug in the runtime. I suggest instead we roll back
> this whole change until we understand better what is going on.
>
> -rob
>

I understand what is going on, but it is impossible for me to completely
fix the issue in a short amount time.

[0xe2.0x9a.0x9b_gmail.com, 2013-04-25 15:11:02]

I will undo the CL, as you suggested.


On Thu, Apr 25, 2013 at 5:04 PM, Rob Pike <r@golang.org> wrote:

> Is your suggest that we roll back a fix that has caught a bug in the
> runtime?
>
> I would prefer you roll back this change, which just replaces one bug
> with another. It's not clear to me that the original bug is a
> show-stopper.
>
> -rob
>

[r, 2013-04-25 15:11:16]

So please roll it back.

[niemeyer, 2013-04-25 15:22:50]

On 2013/04/25 15:04:29, r wrote:
> with another. It's not clear to me that the original bug is a
> show-stopper.

It's perhaps not a show stopper because finalizers are generally a last resort
measure for something that would be better done in a deterministic way. If might
be a show stopper in the sense that SetFinalizer seems to prevent values from
being collected, though, which means people are introducing leaks in their
applications, ironically by trying to avoid resource leaks. I had the same
experience with mgo some time ago, but haven't dived in to see in detail why the
values were not collected. Instead, I just dropped the finalizers.

[dvyukov, 2013-04-25 15:26:22]

This prevents at most *one* object from being collected.

On Thu, Apr 25, 2013 at 7:22 PM,  <n13m3y3r@gmail.com> wrote:
> On 2013/04/25 15:04:29, r wrote:
>>
>> with another. It's not clear to me that the original bug is a
>> show-stopper.
>
>
> It's perhaps not a show stopper because finalizers are generally a last
> resort measure for something that would be better done in a
> deterministic way. If might be a show stopper in the sense that
> SetFinalizer seems to prevent values from being collected, though, which
> means people are introducing leaks in their applications, ironically by
> trying to avoid resource leaks. I had the same experience with mgo some
> time ago, but haven't dived in to see in detail why the values were not
> collected. Instead, I just dropped the finalizers.
>
> https://codereview.appspot.com/8954044/

[gustavo_niemeyer.net, 2013-04-25 15:39:57]

On Thu, Apr 25, 2013 at 12:26 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> This prevents at most *one* object from being collected.

What if that one object holds on to a state with N other objects?

Either way, if you forgive my lack of faith, I'm unsure about things
being in a clean state at this point. I believe there are zero tests
right now that cover the actual functionality of finalizers, and they
are only ever used in file descriptors if people forget to close the
file in the first place.

Sorry, I'm not pointing fingers, and the failed test I did may well be
my fault, but I just don't feel confident at this point.


gustavo @ http://niemeyer.net

[gustavo_niemeyer.net, 2013-04-25 15:51:41]

On Thu, Apr 25, 2013 at 12:39 PM, Gustavo Niemeyer <gustavo@niemeyer.net>
wrote:
> I believe there are zero tests right now that cover the actual
> functionality of finalizers,

To be fair, that's actually not true. There's a test under test/, that
works within a tolerance of 80% of collections.


gustavo @ http://niemeyer.net

[minux1, 2013-04-25 15:58:54]

On Thu, Apr 25, 2013 at 11:39 PM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> On Thu, Apr 25, 2013 at 12:26 PM, Dmitry Vyukov <dvyukov@google.com>
> wrote:
> > This prevents at most *one* object from being collected.
>
> What if that one object holds on to a state with N other objects?
>
> Either way, if you forgive my lack of faith, I'm unsure about things
> being in a clean state at this point. I believe there are zero tests
>
The only functional test for SetFinalizer is:
/test/mallocfin.go
but that test only verify that finalizers are invoked.

> right now that cover the actual functionality of finalizers, and they
> are only ever used in file descriptors if people forget to close the
> file in the first place.

[r, 2013-04-25 16:01:54]

It's a rare enough bug that I'm not too concerned about it, especially
as the fix seems more problematical than the bug.

I prefer to visit this issue after Go 1.1 is launched, rather than
delay Go 1.1 with a protracted sequence of fixes to this subtle
problem.

-rob