code review 88210044: runtime: crash when func main calls Goexit and all othe...

runtime: crash when func main calls Goexit and all other goroutines exit

This has typically crashed in the past, although usually with
an 'all goroutines are asleep - deadlock!' message that shows
no goroutines (because there aren't any).

Previous discussion at:
https://groups.google.com/d/msg/golang-nuts/uCT_7WxxopQ/BoSBlLFzUTkJ
https://groups.google.com/d/msg/golang-dev/KUojayEr20I/u4fp_Ej5PdUJ
http://golang.org/issue/7711

There is general agreement that runtime.Goexit terminates the
main goroutine, so that main cannot return, so the program does
not exit.

The interpretation that all other goroutines exiting causes an
exit(0) is relatively new and was not part of those discussions.
That is what this CL changes.

Thankfully, even though the exit(0) has been there for a while,
some other accounting bugs made it very difficult to trigger,
so it is reasonable to replace. In particular, see golang.org/issue/7711#c10
for an examination of the behavior across past releases.

Fixes issue 7711.

[rsc, 2014-04-16 01:14:33]

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

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

[iant, 2014-04-16 05:34:33]

LGTM

[dvyukov, 2014-04-16 06:15:42]

What is the rationale behind this change?

The current interpretation is not relatively new. It's mentioned in the
discussions that you've referenced. And see e.g.
https://codereview.appspot.com/7422054 which adds an explicit test for this
behavior during Go1.1 dev cycle.

I can see how the current behavior can be useful:
http://play.golang.org/p/-55HemjMTX
compare with uncommented select{}

I can understand if we want to declare this pattern unimportant and then
simplify runtime by removing all that special logic that counts interesting
subset of alive goroutines. Then programs will crash with standard deadlock
message.

I can understand if we equal Goexit and return from main goroutine. But it's
late to do such change.

I do not understand (1) why we want to crash the program in this case (if
anything it does not look like programming error), (2) if we want to crash it,
why we want to complicate scheduler just to print a different error message.

[dvyukov, 2014-04-16 06:17:45]

Does it really fix issue 7711?
I would expect that the program still hangs. And if it does not then, that this
change introduces a bug, because the program must hang (it is potentially
serving callbacks from C).

[rsc, 2014-04-16 14:35:56]

On Wed, Apr 16, 2014 at 2:15 AM, <dvyukov@google.com> wrote:

> What is the rationale behind this change?
>

A Go program exits 0 when main returns. If main does not return and the
program does not call os.Exit(0) explicitly, it should not exit 0. This CL
is only about the exit 0.

When there's nothing left for a Go program to do and we can detect that
fact, the program crashes (the deadlock message). We can also detect no
goroutines at all, so that should crash too, and that's subtle enough that
it will probably avoid confusion if we print a custom message.

In general you should not care whether a package you are using has kicked
off background goroutines. For example, there is a change pending in
package net that will kick off a goroutine to watch /etc/resolv.conf for
configuration changes (after the first time it is read). If you have a
program func main() { net.LookupHost("localhost"); runtime.Goexit() }, then
it exits 0 today but after that net CL lands it will sit forever instead.
That's the kind of difference - success vs failure - that should not depend
on internal package implementation details. (The same basic idea is why the
program exits when main returns. It is also why we allow goroutines during
init. Before we did that you had to avoid using regexps and templates
during init; the goroutines in those packages were an implementation detail
leaking out.)

With my CL, then the net program crashes with a deadlock message today and
after the net CL lands it will sit forever instead. That's at least two
different kinds of failures instead of a success and a failure. And we've
already decided that we would rather crash with a deadlock message than sit
forever when we can detect it, so we're already comfortable with background
goroutines affecting that difference.

Everyone agrees that runtime.Goexit from main keeps main from returning.
But if main doesn't return, then the program should not exit 0. Not at the
call to runtime.Goexit, and not later.


> The current interpretation is not relatively new. It's mentioned in the
> discussions that you've referenced.


Nowhere in the discussions does it say that runtime.Goexit from main should
cause the program to exit 0 once all other goroutines finish. Here are the
discussions:

https://groups.google.com/forum/#!msg/golang-nuts/uCT_7WxxopQ/BoSBlLFzUTkJ

This one talks about a program with a background goroutine that sleeps
forever and a main goroutine that calls runtime.Goexit. You asked if it
should exit. It didn't at the time, and Ian said it shouldn't. I threatened
to change the behavior but did not. The behavior of the program in that
discussion is unchanged by this CL. It sleeps forever. No mention of exit 0
when all the other goroutines finish.

https://groups.google.com/forum/#!msg/golang-dev/KUojayEr20I/u4fp_Ej5PdUJ

In this discussion, you pointed out that after main calls runtime.Goexit,
"the program continues to work while there are other goroutines". Andrey
replied pointing out that "calling runtime.Goexit() in main guarantees that
the program will panic once no other goroutines (but the memory scavenger)
are running." I replied saying that we should keep that behavior. Again, no
mention of exit 0 when all the other goroutines finish.

And then there is http://golang.org/issue/7711, where you mentioned "daemon
mode" in comment 4. That's the very first time anyone has mentioned that
the program does exit 0 when all the other goroutines finish.

And see e.g.
> https://codereview.appspot.com/7422054 which adds an explicit test for
> this behavior during Go1.1 dev cycle.
>

Yes, that does add a test for this behavior. I am sorry I didn't notice
this at the time. Luckily, it seems that because of bugs trying to hide the
runtime's own background goroutines, any real program was crashing instead
of exiting 0. So this CL does not change the behavior on real programs. For
example, look at Rob's tests reported in the issue. There have been no Go
1.x releases when that program exited 0. You said it doesn't count, but
it's much more like a real program than func main() { runtime.Goexit() }.

I can see how the current behavior can be useful:
> http://play.golang.org/p/-55HemjMTX
> compare with uncommented select{}
>

No one is arguing that it can be useful. *Every* behavior can be useful.
But making the success of the program depend on whether one package or
another happens to have created a background goroutine is actively harmful.
And that harm outweighs the utility.

I believe that - except for running defers and freeing the G structure and
stack - runtime.Goexit and select{} should be thought of as exactly the
same thing.


> I can understand if we want to declare this pattern unimportant and then
> simplify runtime by removing all that special logic that counts
> interesting subset of alive goroutines. Then programs will crash with
> standard deadlock message.
>

Crashing with the standard deadlock message is somewhat misleading, since
it will not show any goroutines at all. Yes, it is a deadlock of 0
goroutines, but that's weird enough to warrant a special message.

I do not understand (1) why we want to crash the program in this case
> (if anything it does not look like programming error), (2) if we want to
> crash it, why we want to complicate scheduler just to print a different
> error message.
>

(1) It certainly looks like a programmer error - misuse or misunderstanding
of runtime.Goexit.

(2) Printing about a deadlock of 0 goroutines seems to me confusing enough
to warrant the logic for a special message. And the counting logic is
already there so all I need to do is change 1 line.

Russ

[r, 2014-04-16 15:03:08]

LGTM
probably warrants words in the release notes

[rsc, 2014-04-16 15:36:11]

PTAL: Added doc/go1.3.html to this CL. The new text is:

<li>
If the main goroutine calls
<a href="/pkg/runtime/#Goexit"><code>runtime.Goexit</code>
and all other goroutines finish execution, the program now always crashes,
reporting a detected deadlock.
Earlier versions of Go handled this situation inconsistently: most instances
were reported as deadlocks, but some trivial cases exited cleanly instead.
</li>

[dvyukov, 2014-04-16 15:54:32]

On 2014/04/16 14:35:56, rsc wrote:
> On Wed, Apr 16, 2014 at 2:15 AM, <mailto:dvyukov@google.com> wrote:
> 
> > What is the rationale behind this change?
> >
> 
> A Go program exits 0 when main returns. If main does not return and the
> program does not call os.Exit(0) explicitly, it should not exit 0. This CL
> is only about the exit 0.
> 
> When there's nothing left for a Go program to do and we can detect that
> fact, the program crashes (the deadlock message). We can also detect no
> goroutines at all, so that should crash too, and that's subtle enough that
> it will probably avoid confusion if we print a custom message.
> 
> In general you should not care whether a package you are using has kicked
> off background goroutines. For example, there is a change pending in
> package net that will kick off a goroutine to watch /etc/resolv.conf for
> configuration changes (after the first time it is read). If you have a
> program func main() { net.LookupHost("localhost"); runtime.Goexit() }, then
> it exits 0 today but after that net CL lands it will sit forever instead.
> That's the kind of difference - success vs failure - that should not depend
> on internal package implementation details. (The same basic idea is why the
> program exits when main returns. It is also why we allow goroutines during
> init. Before we did that you had to avoid using regexps and templates
> during init; the goroutines in those packages were an implementation detail
> leaking out.)
> 
> With my CL, then the net program crashes with a deadlock message today and
> after the net CL lands it will sit forever instead. That's at least two
> different kinds of failures instead of a success and a failure. And we've
> already decided that we would rather crash with a deadlock message than sit
> forever when we can detect it, so we're already comfortable with background
> goroutines affecting that difference.
> 
> Everyone agrees that runtime.Goexit from main keeps main from returning.
> But if main doesn't return, then the program should not exit 0. Not at the
> call to runtime.Goexit, and not later.
> 
> 
> > The current interpretation is not relatively new. It's mentioned in the
> > discussions that you've referenced.
> 
> 
> Nowhere in the discussions does it say that runtime.Goexit from main should
> cause the program to exit 0 once all other goroutines finish. Here are the
> discussions:
> 
> https://groups.google.com/forum/#!msg/golang-nuts/uCT_7WxxopQ/BoSBlLFzUTkJ
> 
> This one talks about a program with a background goroutine that sleeps
> forever and a main goroutine that calls runtime.Goexit. You asked if it
> should exit. It didn't at the time, and Ian said it shouldn't. I threatened
> to change the behavior but did not. The behavior of the program in that
> discussion is unchanged by this CL. It sleeps forever. No mention of exit 0
> when all the other goroutines finish.
> 
> https://groups.google.com/forum/#!msg/golang-dev/KUojayEr20I/u4fp_Ej5PdUJ
> 
> In this discussion, you pointed out that after main calls runtime.Goexit,
> "the program continues to work while there are other goroutines". Andrey
> replied pointing out that "calling runtime.Goexit() in main guarantees that
> the program will panic once no other goroutines (but the memory scavenger)
> are running." I replied saying that we should keep that behavior. Again, no
> mention of exit 0 when all the other goroutines finish.
> 
> And then there is http://golang.org/issue/7711, where you mentioned "daemon
> mode" in comment 4. That's the very first time anyone has mentioned that
> the program does exit 0 when all the other goroutines finish.
> 
> And see e.g.
> > https://codereview.appspot.com/7422054 which adds an explicit test for
> > this behavior during Go1.1 dev cycle.
> >
> 
> Yes, that does add a test for this behavior. I am sorry I didn't notice
> this at the time. Luckily, it seems that because of bugs trying to hide the
> runtime's own background goroutines, any real program was crashing instead
> of exiting 0. So this CL does not change the behavior on real programs. For
> example, look at Rob's tests reported in the issue. There have been no Go
> 1.x releases when that program exited 0. You said it doesn't count, but
> it's much more like a real program than func main() { runtime.Goexit() }.
> 
> I can see how the current behavior can be useful:
> > http://play.golang.org/p/-55HemjMTX
> > compare with uncommented select{}
> >
> 
> No one is arguing that it can be useful. *Every* behavior can be useful.
> But making the success of the program depend on whether one package or
> another happens to have created a background goroutine is actively harmful.
> And that harm outweighs the utility.
> 
> I believe that - except for running defers and freeing the G structure and
> stack - runtime.Goexit and select{} should be thought of as exactly the
> same thing.
> 
> 
> > I can understand if we want to declare this pattern unimportant and then
> > simplify runtime by removing all that special logic that counts
> > interesting subset of alive goroutines. Then programs will crash with
> > standard deadlock message.
> >
> 
> Crashing with the standard deadlock message is somewhat misleading, since
> it will not show any goroutines at all. Yes, it is a deadlock of 0
> goroutines, but that's weird enough to warrant a special message.
> 
> I do not understand (1) why we want to crash the program in this case
> > (if anything it does not look like programming error), (2) if we want to
> > crash it, why we want to complicate scheduler just to print a different
> > error message.
> >
> 
> (1) It certainly looks like a programmer error - misuse or misunderstanding
> of runtime.Goexit.
> 
> (2) Printing about a deadlock of 0 goroutines seems to me confusing enough
> to warrant the logic for a special message. And the counting logic is
> already there so all I need to do is change 1 line.


The point about background goroutines affecting behavior makes sense. I am
convinced.

If it's all about a better crash message, can we simply remember that main has
goexited and add this fact to standards deadlock message? The current logic will
break again in future.

[dvyukov, 2014-04-16 15:57:39]

On 2014/04/16 15:54:32, dvyukov wrote:
> On 2014/04/16 14:35:56, rsc wrote:
> > On Wed, Apr 16, 2014 at 2:15 AM, <mailto:dvyukov@google.com> wrote:
> > 
> > > What is the rationale behind this change?
> > >
> > 
> > A Go program exits 0 when main returns. If main does not return and the
> > program does not call os.Exit(0) explicitly, it should not exit 0. This CL
> > is only about the exit 0.
> > 
> > When there's nothing left for a Go program to do and we can detect that
> > fact, the program crashes (the deadlock message). We can also detect no
> > goroutines at all, so that should crash too, and that's subtle enough that
> > it will probably avoid confusion if we print a custom message.
> > 
> > In general you should not care whether a package you are using has kicked
> > off background goroutines. For example, there is a change pending in
> > package net that will kick off a goroutine to watch /etc/resolv.conf for
> > configuration changes (after the first time it is read). If you have a
> > program func main() { net.LookupHost("localhost"); runtime.Goexit() }, then
> > it exits 0 today but after that net CL lands it will sit forever instead.
> > That's the kind of difference - success vs failure - that should not depend
> > on internal package implementation details. (The same basic idea is why the
> > program exits when main returns. It is also why we allow goroutines during
> > init. Before we did that you had to avoid using regexps and templates
> > during init; the goroutines in those packages were an implementation detail
> > leaking out.)
> > 
> > With my CL, then the net program crashes with a deadlock message today and
> > after the net CL lands it will sit forever instead. That's at least two
> > different kinds of failures instead of a success and a failure. And we've
> > already decided that we would rather crash with a deadlock message than sit
> > forever when we can detect it, so we're already comfortable with background
> > goroutines affecting that difference.
> > 
> > Everyone agrees that runtime.Goexit from main keeps main from returning.
> > But if main doesn't return, then the program should not exit 0. Not at the
> > call to runtime.Goexit, and not later.
> > 
> > 
> > > The current interpretation is not relatively new. It's mentioned in the
> > > discussions that you've referenced.
> > 
> > 
> > Nowhere in the discussions does it say that runtime.Goexit from main should
> > cause the program to exit 0 once all other goroutines finish. Here are the
> > discussions:
> > 
> > https://groups.google.com/forum/#!msg/golang-nuts/uCT_7WxxopQ/BoSBlLFzUTkJ
> > 
> > This one talks about a program with a background goroutine that sleeps
> > forever and a main goroutine that calls runtime.Goexit. You asked if it
> > should exit. It didn't at the time, and Ian said it shouldn't. I threatened
> > to change the behavior but did not. The behavior of the program in that
> > discussion is unchanged by this CL. It sleeps forever. No mention of exit 0
> > when all the other goroutines finish.
> > 
> > https://groups.google.com/forum/#!msg/golang-dev/KUojayEr20I/u4fp_Ej5PdUJ
> > 
> > In this discussion, you pointed out that after main calls runtime.Goexit,
> > "the program continues to work while there are other goroutines". Andrey
> > replied pointing out that "calling runtime.Goexit() in main guarantees that
> > the program will panic once no other goroutines (but the memory scavenger)
> > are running." I replied saying that we should keep that behavior. Again, no
> > mention of exit 0 when all the other goroutines finish.
> > 
> > And then there is http://golang.org/issue/7711, where you mentioned "daemon
> > mode" in comment 4. That's the very first time anyone has mentioned that
> > the program does exit 0 when all the other goroutines finish.
> > 
> > And see e.g.
> > > https://codereview.appspot.com/7422054 which adds an explicit test for
> > > this behavior during Go1.1 dev cycle.
> > >
> > 
> > Yes, that does add a test for this behavior. I am sorry I didn't notice
> > this at the time. Luckily, it seems that because of bugs trying to hide the
> > runtime's own background goroutines, any real program was crashing instead
> > of exiting 0. So this CL does not change the behavior on real programs. For
> > example, look at Rob's tests reported in the issue. There have been no Go
> > 1.x releases when that program exited 0. You said it doesn't count, but
> > it's much more like a real program than func main() { runtime.Goexit() }.
> > 
> > I can see how the current behavior can be useful:
> > > http://play.golang.org/p/-55HemjMTX
> > > compare with uncommented select{}
> > >
> > 
> > No one is arguing that it can be useful. *Every* behavior can be useful.
> > But making the success of the program depend on whether one package or
> > another happens to have created a background goroutine is actively harmful.
> > And that harm outweighs the utility.
> > 
> > I believe that - except for running defers and freeing the G structure and
> > stack - runtime.Goexit and select{} should be thought of as exactly the
> > same thing.
> > 
> > 
> > > I can understand if we want to declare this pattern unimportant and then
> > > simplify runtime by removing all that special logic that counts
> > > interesting subset of alive goroutines. Then programs will crash with
> > > standard deadlock message.
> > >
> > 
> > Crashing with the standard deadlock message is somewhat misleading, since
> > it will not show any goroutines at all. Yes, it is a deadlock of 0
> > goroutines, but that's weird enough to warrant a special message.
> > 
> > I do not understand (1) why we want to crash the program in this case
> > > (if anything it does not look like programming error), (2) if we want to
> > > crash it, why we want to complicate scheduler just to print a different
> > > error message.
> > >
> > 
> > (1) It certainly looks like a programmer error - misuse or misunderstanding
> > of runtime.Goexit.
> > 
> > (2) Printing about a deadlock of 0 goroutines seems to me confusing enough
> > to warrant the logic for a special message. And the counting logic is
> > already there so all I need to do is change 1 line.
> 
> 
> The point about background goroutines affecting behavior makes sense. I am
> convinced.

And thanks for the exhaustive explanation.

> If it's all about a better crash message, can we simply remember that main has
> goexited and add this fact to standards deadlock message? The current logic
will
> break again in future.

[r, 2014-04-16 16:15:52]

LGTM

[rsc, 2014-04-16 17:12:02]

On Wed, Apr 16, 2014 at 11:54 AM, <dvyukov@google.com> wrote:

> If it's all about a better crash message, can we simply remember that
> main has goexited and add this fact to standards deadlock message? The
> current logic will break again in future.
>

Let's deal with that when it does (and let's try to keep it not broken).

[rsc, 2014-04-16 17:12:22]

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

runtime: crash when func main calls Goexit and all other goroutines exit

This has typically crashed in the past, although usually with
an 'all goroutines are asleep - deadlock!' message that shows
no goroutines (because there aren't any).

Previous discussion at:
https://groups.google.com/d/msg/golang-nuts/uCT_7WxxopQ/BoSBlLFzUTkJ
https://groups.google.com/d/msg/golang-dev/KUojayEr20I/u4fp_Ej5PdUJ
http://golang.org/issue/7711

There is general agreement that runtime.Goexit terminates the
main goroutine, so that main cannot return, so the program does
not exit.

The interpretation that all other goroutines exiting causes an
exit(0) is relatively new and was not part of those discussions.
That is what this CL changes.

Thankfully, even though the exit(0) has been there for a while,
some other accounting bugs made it very difficult to trigger,
so it is reasonable to replace. In particular, see golang.org/issue/7711#c10
for an examination of the behavior across past releases.

Fixes issue 7711.

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

[gobot, 2014-04-16 17:23:15]

This CL appears to have broken the linux-386-387 builder.
See http://build.golang.org/log/c0e3549fa9374ef5d2d0c8d713780801b2be6148