code review 6454170: runtime: Wait until panics have been printed before exiting.

runtime: Wait until panics have been printed before exiting.

Fixes issue 3934.

[DMorsing, 2012-08-19 14:09:22]

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

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

[DMorsing, 2012-08-19 14:13:00]

It's probably bad form to submit a CL for a bug that hasn't been accepted, but
the bug seemed valid and the fix was fairly trivial

[brainman, 2012-08-20 23:33:32]

On Tuesday, 21 August 2012 07:07:36 UTC+10, Daniel Morsing wrote:
> On Mon, Aug 20, 2012 at 12:24 AM, brainman <alex.b...@gmail.com> wrote:
> > Can you write a test for it?
> >
> > Alex
> 
> I've tried, but i can't find a way to make one that works with go's
> testing library.

I assume that you could write a little go program to demonstrate the bug. Then
you could run this program from inside your test and verify that it behaves in a
particular way (prints expected message or something). You could see something
similar in src/pkg/runtime/crash_test.go, but, I suspect, your test will be even
simpler.

Alex

[DMorsing, 2012-08-21 06:18:08]

On Tue, Aug 21, 2012 at 1:33 AM,  <alex.brainman@gmail.com> wrote:
> I assume that you could write a little go program to demonstrate the
> bug. Then you could run this program from inside your test and verify
> that it behaves in a particular way (prints expected message or
> something). You could see something similar in
> src/pkg/runtime/crash_test.go, but, I suspect, your test will be even
> simpler.

The big problem I have is figuring out how to evaluate success. The
truncated part of the panic trace is dependent on the path that go was
compiled in, so I can't test it against a known good version.

[brainman, 2012-08-21 06:26:22]

On 2012/08/21 06:18:08, DMorsing wrote:
> 
> The big problem I have is figuring out how to evaluate success. The
> truncated part of the panic trace is dependent on the path that go was
> compiled in, so I can't test it against a known good version.

Perhaps I do not understand what your problem is, but anything "close enough" is
better then no test. Perhaps, you could use regex to match last line of your
output. Or something similar.

Alex

[DMorsing, 2012-08-27 17:30:19]

I figured out how to test this change pretty trivially. Just check the
return value of the program. If it's 2, it exited from the panic and
if it's 0, it exited from main.

However, there's no support for checking for the explicit return value
of a test in run.go, and I have a feeling that tests that use bash
scripting isn't welcome in new code.

I would add it to run.go and testlib, but I'm not sure what would be a
good syntax to use for this feature. Any suggestions?

[brainman, 2012-08-28 01:48:52]

On 2012/08/27 17:30:19, DMorsing wrote:
> I figured out how to test this change pretty trivially. Just check the
> return value of the program. If it's 2, it exited from the panic and
> if it's 0, it exited from main.
> 

Sounds good.

> However, there's no support for checking for the explicit return value
> of a test in run.go, and I have a feeling that tests that use bash
> scripting isn't welcome in new code.
> 
> I would add it to run.go and testlib, but I'm not sure what would be a
> good syntax to use for this feature. Any suggestions?

I do not think you should fiddle with test/run.go. Just create new test in
runtime, like src/pkg/runtime/crash_test.go.

Alex

[DMorsing, 2012-08-28 17:37:30]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2012-08-29 01:20:58]

Your new test fails

# go test -v -run=Panic
=== RUN TestPanicWhileReturning
--- FAIL: TestPanicWhileReturning (0.06 seconds)
panic_test.go:33:       program exited without error. Output:
                FAIL
exit status 1
FAIL    runtime 0.069s

on my linux/386 - hg id is 3b78b41a4b50+ tip. It is single cpu, if it makes any
difference.

The same test PASS on my windows/386 pc.

Alex

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/panic_test.go
File src/pkg/runtime/panic_test.go (right):

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/panic_test.g...
src/pkg/runtime/panic_test.go:25: defer os.Remove(f.Name())
defer os.Remove ...

line should go before

defer f.Close()

os.Remove while file is opened will work on unix, but will fail on windows. You
do not want to leave files in people tmp directories.

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:249: static Lock deadlock;
Do you really need this variable to be "static"?

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:249: static Lock deadlock;
Russ likes all local variables to be defined at the start of function.

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:251: runtime·lock(&deadlock);
Why two lock lines?

[DMorsing, 2012-08-29 15:54:02]

The test failure is a bit surprising. It worked fine on this machine, failing on
go 1.0.2 and passing on tip+CL.

My immediate instinct would be that the os/exec package is sourcing the go
command used in the test from somewhere other than the shell.

Anyway, PTAL

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/panic_test.go
File src/pkg/runtime/panic_test.go (right):

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/panic_test.g...
src/pkg/runtime/panic_test.go:25: defer os.Remove(f.Name())
On 2012/08/29 01:20:58, brainman wrote:
> defer os.Remove ...
> 
> line should go before
> 
> defer f.Close()
> 
> os.Remove while file is opened will work on unix, but will fail on windows.
You
> do not want to leave files in people tmp directories.

Done.

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:249: static Lock deadlock;
On 2012/08/29 01:20:58, brainman wrote:
> Do you really need this variable to be "static"?

This part of the CL was taken almost verbatim from the dopanic function in
panic.c. I figured that was the right way to do things.

http://codereview.appspot.com/6454170/diff/14001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:251: runtime·lock(&deadlock);
On 2012/08/29 01:20:58, brainman wrote:
> Why two lock lines?

The purpose of this sequence is to deadlock the current thread. When a panic is
being printed, it will end by calling exit. So, here we can just deadlock and
let the panics exit call do the work.

[brainman, 2012-08-30 00:26:57]

On 2012/08/29 15:54:02, DMorsing wrote:
> The test failure is a bit surprising. ...

I do not know enough about runtime to comment. But what I am seeing here is your
test program exits "normally", because wg.Wait() exits, because defer wg.Done()
line in foo is executed.

> ... It worked fine on this machine, failing on
> go 1.0.2 and passing on tip+CL.

It works here on another pc of mine (windows/386) too.

> My immediate instinct would be that the os/exec package is sourcing the go
> command used in the test from somewhere other than the shell.

Perhaps, but I do not think so.

> On 2012/08/29 01:20:58, brainman wrote:
> > Why two lock lines?
> 
> The purpose of this sequence is to deadlock the current thread. When a panic
is
> being printed, it will end by calling exit. So, here we can just deadlock and
> let the panics exit call do the work.

Sure. My bad here. I understand the code now - first lock locks it, second lock
blocks on it.

Alex

[DMorsing, 2012-08-30 08:07:06]

On 2012/08/30 00:26:57, brainman wrote:
> I do not know enough about runtime to comment. But what I am seeing here is
your
> test program exits "normally", because wg.Wait() exits, because defer
wg.Done()
> line in foo is executed.

After thinking a bit about it, I think I've found the problem. There is time
between when a panic is caused and when it's printed. In this time, the panic
flag is not set. I'm guessing main returns within that  period on your machine.

You could fix this by saying that main should wait until all panics  has been
handled before returning, even if they don't reach the top of the stack. I'm a
bit cautious to that approach. That seems like it's overprotecting the user from
making mistakes by returning from main.

[brainman, 2012-08-31 01:12:40]

On 2012/08/30 08:07:06, DMorsing wrote:
> 
> ...

I am not right person to make judgment about this, but I think you have a
problem with this patch. Your test fails in some instances and succeeds in
others. It, obviously not a very hood test. Furthermore, as we go further, it
gets less clear to me what the problem is:

- perhaps your test is not good (perhaps it is racy);
- perhaps your runtime fix is not correct;
- perhaps we do not have a problem here - perhaps it is impossible to guarantee
that unhandled panic will print full stack trace;

I suggest you wait for Russ to get around to this ...

Alex

[DMorsing, 2012-08-31 15:32:17]

Test is failing on my machine now, so closing this, and letting it lie until
someone who knows what they're doing comes along and fixes it.

[DMorsing, 2012-08-31 15:32:54]

*** Abandoned ***