code review 4437091: os: add Process.Kill and Process.Signal

os: add Process.Kill and Process.Signal

[eds, 2011-05-03 05:55:04]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[brainman, 2011-05-03 07:51:03]

LGTM. Thank you.

http://codereview.appspot.com/4437091/diff/4001/src/pkg/os/exec_windows.go
File src/pkg/os/exec_windows.go (right):

http://codereview.appspot.com/4437091/diff/4001/src/pkg/os/exec_windows.go#ne...
src/pkg/os/exec_windows.go:37: // Terminate requests that the Process exit. The
Process may ignore this request.
Your comment is misleading here. TerminateProcess can't be ignored on Windows.
Process will terminate immediately. Not sure what to do about the comment.

http://codereview.appspot.com/4437091/diff/4001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4437091/diff/4001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:140: //sys	TerminateProcess(handle uint32,
exitcode uint32) (errno int)
s/handle uint32/handle int32/
CreateProcess returns (handle int32), let's have (handle int32) here too.

[eds, 2011-05-03 08:13:05]

On Tue, May 3, 2011 at 7:51 PM,  <alex.brainman@gmail.com> wrote:
>
http://codereview.appspot.com/4437091/diff/4001/src/pkg/os/exec_windows.go#ne...
> src/pkg/os/exec_windows.go:37: // Terminate requests that the Process
> exit. The Process may ignore this request.
> Your comment is misleading here. TerminateProcess can't be ignored on
> Windows. Process will terminate immediately. Not sure what to do about
> the comment.

The comment may be unclear, but I don't think it's inaccurate. Would
it be better if I change it to "It is possible that the Process will
ignore this request."?

>
http://codereview.appspot.com/4437091/diff/4001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:140: //sys   TerminateProcess(handle
> uint32, exitcode uint32) (errno int)
> s/handle uint32/handle int32/
> CreateProcess returns (handle int32), let's have (handle int32) here
> too.

I copied the uint32 from GetExitCodeProcess just below. Want me to
change that one too?

- Evan

[brainman, 2011-05-03 11:38:43]

On 2011/05/03 08:13:05, eds wrote:
> 
> I copied the uint32 from GetExitCodeProcess just below. Want me to
> change that one too?
> 

Please do. Change OpenProcess while you're there too. Thank you.

Alex

[r, 2011-05-03 18:47:05]

http://codereview.appspot.com/4437091/diff/4001/src/pkg/os/exec_unix.go
File src/pkg/os/exec_unix.go (right):

http://codereview.appspot.com/4437091/diff/4001/src/pkg/os/exec_unix.go#newco...
src/pkg/os/exec_unix.go:58: // action.
i think this is a peculiar pair of functions. you're clearly echoing unix
semantics, which makes sense, but if that's what's wanted, why not make it real
unix semantics and just provide (perhaps an alias for) the signal type?

func (p *Process) Signal(int)

i'm happy to listen to counterarguments. unix signals are themselves a mess.

[eds, 2011-05-03 20:37:21]

On Wed, May 4, 2011 at 6:47 AM,  <r@golang.org> wrote:
> i think this is a peculiar pair of functions. you're clearly echoing
> unix semantics, which makes sense, but if that's what's wanted, why not
> make it real unix semantics and just provide (perhaps an alias for) the
> signal type?

Signals really don't translate well to Windows. I think the only
signal it can reasonably emulate is SIGKILL. I don't feel like it's
worth having a Signal method that can only take two different signals,
but maybe others disagree. I'm also open to different names on the two
methods I have if that's an issue.

- Evan

[r2, 2011-05-03 21:22:46]

On May 3, 2011, at 1:37 PM, Evan Shaw wrote:

> On Wed, May 4, 2011 at 6:47 AM,  <r@golang.org> wrote:
>> i think this is a peculiar pair of functions. you're clearly echoing
>> unix semantics, which makes sense, but if that's what's wanted, why not
>> make it real unix semantics and just provide (perhaps an alias for) the
>> signal type?
> 
> Signals really don't translate well to Windows. I think the only
> signal it can reasonably emulate is SIGKILL. I don't feel like it's
> worth having a Signal method that can only take two different signals,
> but maybe others disagree. I'm also open to different names on the two
> methods I have if that's an issue.

I think it makes sense for there to be a Signal (or whatever it's called) to
provide access to Unix-like signalling.  If Windows can only handle a subset,
that's a better situation that forcing Windows-only options onto Unix.

Or it could be argued that if you want Unix signals, use syscall, I suppose. 
But I'm not happy with that.

-rob

[eds, 2011-05-03 21:31:17]

On Wed, May 4, 2011 at 9:22 AM, Rob 'Commander' Pike <r@google.com> wrote:
> I think it makes sense for there to be a Signal (or whatever it's called) to
provide access to Unix-like signalling.  If Windows can only handle a subset,
that's a better situation that forcing Windows-only options onto Unix.

The more I think about it the more reasonable this sounds. Windows can
just return a "not supported" error for most signals.

- Evan

[r2, 2011-05-03 21:32:01]

On May 3, 2011, at 2:31 PM, Evan Shaw wrote:

> On Wed, May 4, 2011 at 9:22 AM, Rob 'Commander' Pike <r@google.com> wrote:
>> I think it makes sense for there to be a Signal (or whatever it's called) to
provide access to Unix-like signalling.  If Windows can only handle a subset,
that's a better situation that forcing Windows-only options onto Unix.
> 
> The more I think about it the more reasonable this sounds. Windows can
> just return a "not supported" error for most signals.

SGTM

[rsc1, 2011-05-03 21:32:14]

There's already a signal.Signal type; maybe it
should be moved into os (from os/signal).

[brainman, 2011-05-03 23:27:18]

On 2011/05/03 21:22:46, r2 wrote:
> 
> I think it makes sense for there to be a Signal (or whatever it's called) to
> provide access to Unix-like signalling.  If Windows can only handle a subset,
> that's a better situation that forcing Windows-only options onto Unix.
> 

Sounds reasonable. But I think we should also have a function to terminate
process.

func (p *Process) Kill() Error

or

func (p *Process) Terminate() Error

I'm not fussy which name to use. Windows users have no concept of "sending
signal to process to terminate it", they just call appropriate Windows api
TerminateProcess. Unix version of this function will just send KILL to process.
We can even put that description in the function doco.

Alex

[eds, 2011-05-08 07:39:16]

Hello golang-dev@googlegroups.com, brainman, r, r2, rsc1 (cc:
golang-dev@googlegroups.com),

Please take another look.

[eds, 2011-05-08 07:41:09]

Sorry, I've been slow on this one.

I've attempted a compromise: kept Kill, removed Terminate, and added
Signal. Kill is just a convenience function for Signal(SIGKILL). The
signal constants I've added to os are all POSIX, but aren't
exhaustive. Suggestions for including or excluding signal constants in
os are welcome.

I haven't done anything with os/signal because I'm not quite sure how
it fits in with what I'm doing here. I don't think it makes sense to
have Process.Signal take a Signal interface parameter, but maybe
there's a benefit I don't see.

- Evan

[brainman, 2011-05-08 10:16:14]

LGTM. You should change CL description, but, please, wait until all comments are
heard. Thank you.

Alex

[eds, 2011-05-22 01:30:35]

Ping. Any thoughts?

I've also updated the CL description.

- Evan

[krasin, 2011-05-26 17:21:04]

(just a side comment)

http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_windows.go
File src/pkg/os/exec_windows.go (right):

http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_windows.go#n...
src/pkg/os/exec_windows.go:35: e := syscall.TerminateProcess(int32(p.handle),
137)
Where 137 comes from? Could you please point to the documentation / blog article
about that?

[rsc, 2011-05-26 17:25:26]

http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_unix.go
File src/pkg/os/exec_unix.go (right):

http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_unix.go#newc...
src/pkg/os/exec_unix.go:48: // Signal sends a signal to the Process.
Please take the types from os/signal and move them into os.
Then the argument here should be os.Signal

[eds, 2011-05-26 20:55:19]

On Fri, May 27, 2011 at 5:21 AM,  <krasin@google.com> wrote:
>
http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_windows.go#n...
> src/pkg/os/exec_windows.go:35: e :=
> syscall.TerminateProcess(int32(p.handle), 137)
> Where 137 comes from? Could you please point to the documentation / blog
> article about that?

This might actually just be a Linux convention rather than Unix. I
don't have another system to test. POSIX says:

"If the process terminated abnormally due to the receipt of a signal,
the exit status shall be greater than 128 and shall be distinct from
the exit status generated by other signals, but the exact value is
unspecified." (http://pubs.opengroup.org/onlinepubs/9699919799/)

The Advanced Bash-Scripting Guide says the exit status when a process
is killed by a signal is 128+signal number.
(http://tldp.org/LDP/abs/html/exitcodes.html)

I'll change it to say 128+SIGKILL to make it a little clearer.

- Evan

[eds, 2011-05-26 20:57:22]

On Fri, May 27, 2011 at 5:25 AM,  <rsc@golang.org> wrote:
> Please take the types from os/signal and move them into os.
> Then the argument here should be os.Signal

I need some convincing. Won't Process.Signal just end up doing a type
switch or type assertion? If the method is just looking for one
particular implementation or set of implementations, then why not just
make it a concrete type instead?

The Signal interface makes sense to me when you're getting a Signal
out of a package, but not when you're giving one as input.

- Evan

[rsc, 2011-05-26 21:01:10]

It's an interface because it can be different concrete type
on different systems, not because we expect someone
else to implement the interface.  If p.Signal is passed
something that's not a UnixSignal then it can panic.
But the signature needs to be the same on all systems,
so os.Signal as the type.

[iant2, 2011-05-26 21:21:43]

Evan Shaw <chickencha@gmail.com> writes:

> On Fri, May 27, 2011 at 5:21 AM,  <krasin@google.com> wrote:
>>
http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_windows.go#n...
>> src/pkg/os/exec_windows.go:35: e :=
>> syscall.TerminateProcess(int32(p.handle), 137)
>> Where 137 comes from? Could you please point to the documentation / blog
>> article about that?
>
> This might actually just be a Linux convention rather than Unix. I
> don't have another system to test. POSIX says:
>
> "If the process terminated abnormally due to the receipt of a signal,
> the exit status shall be greater than 128 and shall be distinct from
> the exit status generated by other signals, but the exact value is
> unspecified." (http://pubs.opengroup.org/onlinepubs/9699919799/)
>
> The Advanced Bash-Scripting Guide says the exit status when a process
> is killed by a signal is 128+signal number.
> (http://tldp.org/LDP/abs/html/exitcodes.html)
>
> I'll change it to say 128+SIGKILL to make it a little clearer.

Essentially all Unix systems use the same approach.  But I'm not sure it
makes sense to use a Unix convention on Windows.

Ian

[eds, 2011-05-26 21:46:33]

On Fri, May 27, 2011 at 9:21 AM, Ian Lance Taylor <iant@google.com> wrote:
> Essentially all Unix systems use the same approach.  But I'm not sure it
> makes sense to use a Unix convention on Windows.

That's fair, but I don't know of a number that would be better. If
someone can come up with one, I'm happy to change it. The only thing I
can say with some certainty is that the number should be non-zero.

- Evan

[rsc1, 2011-05-26 21:56:16]

1 would be better than 137.

[eds, 2011-05-27 22:03:18]

Hello alex.brainman@gmail.com, r@golang.org, rsc@golang.org, krasin@google.com,
iant@google.com, rsc@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-06-03 17:02:32]

LGTM

http://codereview.appspot.com/4437091/diff/37002/src/pkg/os/exec_windows.go
File src/pkg/os/exec_windows.go (right):

http://codereview.appspot.com/4437091/diff/37002/src/pkg/os/exec_windows.go#n...
src/pkg/os/exec_windows.go:32: switch sig.(UnixSignal) {
Eventually this should be windows signals (of which there are only one or two).

[rsc1, 2011-06-03 17:05:07]

Please hg sync/hg mail

[eds, 2011-06-06 05:30:46]

Hello alex.brainman@gmail.com, r@golang.org, rsc@golang.org, krasin@google.com,
iant@google.com, rsc@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-06-06 06:13:34]

http://codereview.appspot.com/4437091/diff/39012/src/pkg/os/exec_windows.go
File src/pkg/os/exec_windows.go (right):

http://codereview.appspot.com/4437091/diff/39012/src/pkg/os/exec_windows.go#n...
src/pkg/os/exec_windows.go:34: // exit with 137 since that's what Unix would
return
I don't think this comment matching what you're actually doing.

[eds, 2011-06-06 06:19:46]

Hello alex.brainman@gmail.com, r@golang.org, rsc@golang.org, krasin@google.com,
iant@google.com, rsc@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-06-06 06:21:52]

LGTM

[r2, 2011-06-06 07:01:16]

signal_test.go:17: undefined: SIGHUP
signal_test.go:18: undefined: SIGHUP

building on darwin amd64

[eds, 2011-06-06 08:36:27]

Hello alex.brainman@gmail.com, r@golang.org, rsc@golang.org, krasin@google.com,
iant@google.com, rsc@google.com, r@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2011-06-06 09:50:25]

LGTM
although i expect trouble. one way to find out.

[r, 2011-06-06 09:53:36]

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

os: add Process.Kill and Process.Signal

R=alex.brainman, r, rsc, krasin, iant, rsc, r
CC=golang-dev
http://codereview.appspot.com/4437091

Committer: Rob Pike <r@golang.org>