code review 199082: os/signal: send SIGCHLDs to Incoming

os/signal: send SIGCHLDs to Incoming

[cw, 2010-02-03 09:01:21]

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

I'd like you to review this change.

[dsymonds, 2010-02-03 09:03:29]

LGTM

[cw, 2010-02-03 09:19:27]

Actually... this won't work as-is.

I just noticed time.Sleep() is terminating faster than expected because of
interrupted system calls.

We do have Q+R and so we should be setting SA_RESTART.  I guess I need to
investigate more.

[rsc, 2010-02-03 09:21:28]

all.bash fails with this change.

[cw, 2010-02-03 09:25:30]

On Wed, Feb 03, 2010 at 01:21:21AM -0800, Russ Cox wrote:

> all.bash fails with this change.

on what platform?

amd64 shows signal restart not working, i'm not sure why yet, at a
glance the code looks sane

[rsc, 2010-02-03 09:28:29]

>
> > all.bash fails with this change.
>
> on what platform?
>

i would imagine all of them, since it's the same change
on all platforms, but i specifically checked darwin/amd64.
the change treats SIGCHLD the same as SIGTERM,
which is certainly wrong.

russ

[rsc, 2010-02-03 09:31:45]

more generally i don't understand the rationale behind
using SIGCHLD when you can just start a new
goroutine running os.Wait.  the latter strategy will
even run without modification on Windows (some day).

russ

[cw, 2010-02-03 09:32:21]

On Wed, Feb 03, 2010 at 01:28:17AM -0800, Russ Cox wrote:

> the change treats SIGCHLD the same as SIGTERM,
> which is certainly wrong.

indeed, that seems wrong

when the restart issue is resolve SIGTERM will need a different
behavior, but what that should be will depend on the application

i think we'll need to add a function or two to os/signal so
applications can decide what the default signal behavior should be

let me sleep on this

sorry for the noise

[cw, 2010-02-03 09:41:40]

*** Abandoned ***

[rsc, 2010-02-03 09:42:45]

>
> when the restart issue is resolve SIGTERM will need a different
> behavior, but what that should be will depend on the application
>

I'm not sure what the restart issue is, but time.Sleep getting
interrupted is just a bug in time.Sleep (it's on the issue tracker).
There's nothing you can do in the signal setup to keep
time.Sleep from being interrupted by the signal, I don't think.

i think we'll need to add a function or two to os/signal so
> applications can decide what the default signal behavior should be
>

I hope we can avoid that.  The idea behind the channel was that
the app takes over the channel and with it full responsibility
for all signals, including SIGTERM and SIGCHLD and such.
I'd rather not have a second layer of configurability on top of
that or below it.  Signals are a global resource that need to be
owned by one piece of code.  Attempts to pretend otherwise
always seem to end up even more complicated.

If the goal is to get SIGCHLD queued on the channel, it should
suffice to make it Q+I+R, which means that if there is no signal
it's still ignored (not a cause for an exit) but if there's a signal
channel it gets queued.

Russ

[cw, 2010-02-03 09:59:13]

On Wed, Feb 03, 2010 at 01:42:41AM -0800, Russ Cox wrote:

> There's nothing you can do in the signal setup to keep time.Sleep
> from being interrupted by the signal, I don't think.

yes

> The idea behind the channel was that the app takes over the channel
> and with it full responsibility for all signals, including SIGTERM
> and SIGCHLD and such.

i'm fine with that, but then there is a behavior change from what we
do now

[rsc, 2010-02-03 10:07:23]

>
> > The idea behind the channel was that the app takes over the channel
> > and with it full responsibility for all signals, including SIGTERM
> > and SIGCHLD and such.
>
> i'm fine with that, but then there is a behavior change from what we
> do now
>

it's okay if programs that import "os/signal" see another signal.
they asked for it.  i think that's the only behavior change in this CL.

[cw, 2010-02-04 06:31:21]

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

I'd like you to review this change.

[rsc, 2010-02-04 06:38:54]

looks good but please update the description (hg change 199082)
so that the first line works as a standalone line.
for example,

    os/signal: send SIGCHLDs to Incoming

[cw, 2010-02-04 07:00:32]

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

Please take another look.

[rsc, 2010-02-04 07:24:13]

LGTM

looks great, thanks.

[rsc, 2010-02-04 07:24:33]

*** Submitted as http://code.google.com/p/go/source/detail?r=8034bd90d762 ***

os/signal: send SIGCHLDs to Incoming

R=rsc
CC=golang-dev
http://codereview.appspot.com/199082

Committer: Russ Cox <rsc@golang.org>