code review 4029053: os: implement new Process api

os: implement new Process api

Fixes issue 1004.
Fixes issue 1460.

[brainman, 2011-02-02 04:10:19]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[mattn, 2011-02-02 04:28:04]

LGTM

cool!

On 2011/02/02 04:10:19, brainman wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change.

[r, 2011-02-02 05:28:23]

http://codereview.appspot.com/4029053/diff/2001/src/pkg/exec/exec.go
File src/pkg/exec/exec.go (right):

http://codereview.appspot.com/4029053/diff/2001/src/pkg/exec/exec.go#newcode25
src/pkg/exec/exec.go:25: // Process is representing the running command.
I thought Cmd represents the running command

maybe

Process represents the underlying operating system process.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/exec/exec.go#newcode150
src/pkg/exec/exec.go:150: // The options are passed through to p.Process.Wait.
returning the Waitmsg returned when the process exits.
The options are passed to the process's Wait method.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/exec/exec.go#newcode154
src/pkg/exec/exec.go:154: func (p *Cmd) Wait(options int) (*os.Waitmsg,
os.Error) {
s/p/c/ throughout and fix the comments as appropriate.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go
File src/pkg/os/exec.go (right):

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode11
src/pkg/os/exec.go:11: // Process stores the information about monitored os
process.
// Process stores the information about a process created by StartProcess.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode17
src/pkg/os/exec.go:17: // StartProcess starts new process with the program,
arguments,
s/new/a new/

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode18
src/pkg/os/exec.go:18: // and environment specified by name, argv, and envv.  If
successful, Wait method on
This is a poor sentence.  All you need is:

The Wait method of the returned process can be used to wait for it to exit.

(and i'm not even sure you need that, since it's almost a tautology.)

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode20
src/pkg/os/exec.go:20: // exit reason. StartProcess also returns Error, if any. 
The fd array specifies the
StartProcess returns an error if the process cannot be created.
also this sentence should be last.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode46
src/pkg/os/exec.go:46: // ForkExec is deprecated and will be removed after the
next release.
"next release" isn't specific.

// ForkExec is an old function analogous to StartProcess.  It is deprecated and
// will be removed soon.

why not just remove it now?

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode88
src/pkg/os/exec.go:88: // Just a shortcut for os.FindProcess() +
(*Process).Wait() + (*Process).Close().
Wait is a shortcut for p:=os.FindProcess(); p.Wait(); p.Close();

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

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec_unix.go#newco...
src/pkg/os/exec_unix.go:42: // Close releases resources assosiated with the
Process,
s/assosiated/associated/

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec_unix.go#newco...
src/pkg/os/exec_unix.go:43: // rendering it unusable for waiting or quering.
quering??

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec_unix.go#newco...
src/pkg/os/exec_unix.go:50: // FundProcess looks for running process by its pid.
It returns
s/Fund/Find/
s/running/a &/
s/pid/process id, pid/
then
The Process it returns can be used to obtain information about the underlying
operating system process.   If the process cannot be found, an error is
returned.

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

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec_windows.go#ne...
src/pkg/os/exec_windows.go:28: return ErrorString("os: invalid use of
Process.Close")
negative handle in Process.Close? (not sure)
"invalid use" isn't too informative

http://codereview.appspot.com/4029053/diff/2001/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4029053/diff/2001/src/pkg/syscall/exec_unix.go#...
src/pkg/syscall/exec_unix.go:314: // Invented interface to ForkExec to support
what package os expects.
what does Invented mean?

[brainman, 2011-02-02 06:50:35]

Thank you for review. Sorry for my English, but, I assure, I'm as bad in my
native language <g>.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/exec/exec.go
File src/pkg/exec/exec.go (right):

http://codereview.appspot.com/4029053/diff/2001/src/pkg/exec/exec.go#newcode25
src/pkg/exec/exec.go:25: // Process is representing the running command.
On 2011/02/02 05:28:23, r wrote:
> I thought Cmd represents the running command
> 
> maybe
> 
> Process represents the underlying operating system process.

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/exec/exec.go#newcode150
src/pkg/exec/exec.go:150: // The options are passed through to p.Process.Wait.
On 2011/02/02 05:28:23, r wrote:
> returning the Waitmsg returned when the process exits.
> The options are passed to the process's Wait method.

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/exec/exec.go#newcode154
src/pkg/exec/exec.go:154: func (p *Cmd) Wait(options int) (*os.Waitmsg,
os.Error) {
On 2011/02/02 05:28:23, r wrote:
> s/p/c/ throughout and fix the comments as appropriate.

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go
File src/pkg/os/exec.go (right):

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode11
src/pkg/os/exec.go:11: // Process stores the information about monitored os
process.
On 2011/02/02 05:28:23, r wrote:
> // Process stores the information about a process created by StartProcess.

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode17
src/pkg/os/exec.go:17: // StartProcess starts new process with the program,
arguments,
On 2011/02/02 05:28:23, r wrote:
> s/new/a new/

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode18
src/pkg/os/exec.go:18: // and environment specified by name, argv, and envv.  If
successful, Wait method on
On 2011/02/02 05:28:23, r wrote:
> This is a poor sentence.  All you need is:
> 
> The Wait method of the returned process can be used to wait for it to exit.
> 
> (and i'm not even sure you need that, since it's almost a tautology.)

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode20
src/pkg/os/exec.go:20: // exit reason. StartProcess also returns Error, if any. 
The fd array specifies the
On 2011/02/02 05:28:23, r wrote:
> StartProcess returns an error if the process cannot be created.
> also this sentence should be last.

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode46
src/pkg/os/exec.go:46: // ForkExec is deprecated and will be removed after the
next release.
On 2011/02/02 05:28:23, r wrote:
> "next release" isn't specific.
> 
> // ForkExec is an old function analogous to StartProcess.  It is deprecated
and
> // will be removed soon.
> 
> why not just remove it now?

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec.go#newcode88
src/pkg/os/exec.go:88: // Just a shortcut for os.FindProcess() +
(*Process).Wait() + (*Process).Close().
On 2011/02/02 05:28:23, r wrote:
> Wait is a shortcut for p:=os.FindProcess(); p.Wait(); p.Close();

Done.

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

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec_unix.go#newco...
src/pkg/os/exec_unix.go:42: // Close releases resources assosiated with the
Process,
On 2011/02/02 05:28:23, r wrote:
> s/assosiated/associated/

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec_unix.go#newco...
src/pkg/os/exec_unix.go:43: // rendering it unusable for waiting or quering.
On 2011/02/02 05:28:23, r wrote:
> quering??

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec_unix.go#newco...
src/pkg/os/exec_unix.go:50: // FundProcess looks for running process by its pid.
It returns
On 2011/02/02 05:28:23, r wrote:
> s/Fund/Find/
> s/running/a &/
> s/pid/process id, pid/
> then
> The Process it returns can be used to obtain information about the underlying
> operating system process.   If the process cannot be found, an error is
> returned.

Done.

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

http://codereview.appspot.com/4029053/diff/2001/src/pkg/os/exec_windows.go#ne...
src/pkg/os/exec_windows.go:28: return ErrorString("os: invalid use of
Process.Close")
On 2011/02/02 05:28:23, r wrote:
> negative handle in Process.Close? (not sure)
> "invalid use" isn't too informative

Done.

http://codereview.appspot.com/4029053/diff/2001/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4029053/diff/2001/src/pkg/syscall/exec_unix.go#...
src/pkg/syscall/exec_unix.go:314: // Invented interface to ForkExec to support
what package os expects.
On 2011/02/02 05:28:23, r wrote:
> what does Invented mean?

Done.

[brainman, 2011-02-02 06:50:47]

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

Please take another look.

[niemeyer, 2011-02-02 13:10:13]

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

http://codereview.appspot.com/4029053/diff/10001/src/pkg/os/exec_windows.go#n...
src/pkg/os/exec_windows.go:26: func (p *Process) Close() Error {
Can we call this Release rather than Close?

Find + Close feels like doing an action on what was
found, rather than simply giving back resources which
were used during the procedure itself.

Release will also reflect well the fact that
StartProcess + Close doesn't really stop the process
from running, which is a reasonable interpretation.

[rog, 2011-02-02 14:35:06]

how limited are handles as a resource in windows?
if there are no strict limits, maybe just closing
the handle in the finalizer might be sufficient.

a quick google suggests that this might be plausible.

On 2 February 2011 13:10,  <n13m3y3r@gmail.com> wrote:
>
> http://codereview.appspot.com/4029053/diff/10001/src/pkg/os/exec_windows.go
> File src/pkg/os/exec_windows.go (right):
>
>
http://codereview.appspot.com/4029053/diff/10001/src/pkg/os/exec_windows.go#n...
> src/pkg/os/exec_windows.go:26: func (p *Process) Close() Error {
> Can we call this Release rather than Close?
>
> Find + Close feels like doing an action on what was
> found, rather than simply giving back resources which
> were used during the procedure itself.
>
> Release will also reflect well the fact that
> StartProcess + Close doesn't really stop the process
> from running, which is a reasonable interpretation.
>
> http://codereview.appspot.com/4029053/
>

[brainman, 2011-02-03 00:47:37]

On 2011/02/02 13:10:13, niemeyer wrote:
> Can we call this Release rather than Close?
> 

Done.

[brainman, 2011-02-03 00:47:40]

Hello golang-dev@googlegroups.com, mattn, r, niemeyer, rog (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-02-03 01:06:08]

On 2011/02/02 14:35:06, rog wrote:
> how limited are handles as a resource in windows?

I don't know.

> if there are no strict limits, maybe just closing
> the handle in the finalizer might be sufficient.

I'm sure, it is possible, but why hide it, it is just one extra line of code.
Plus I like to have control over resources I hold. I see os.Process api to be
similar to os.File - you open it, you work with it, you tell os that you
finished with it.

> a quick google suggests that this might be plausible.

I'm not hell bent on it. If other reviewers agree with your suggestion, I will
change.

Alex

[rsc, 2011-02-03 03:31:15]

LGTM

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec.go
File src/pkg/os/exec.go (right):

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec.go#newcode11
src/pkg/os/exec.go:11: // Process stores the information about a process created
by StartProcess.
I wonder if this should be per-OS.
Fine for now.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec.go#newcode23
src/pkg/os/exec.go:23: // StartProcess returns an error if the process cannot be
created.
Can delete this (should go without saying).
I realize that it was in the original; that was my fault.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec.go#newcode77
src/pkg/os/exec.go:77: // Wait is a shortcut for p:=os.FindProcess(); p.Wait();
p.Release();
// Wait is equivalent to calling FindProcess and then Wait
// and Release on the result.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec.go#newcode84
src/pkg/os/exec.go:84: return p.Wait(0)
s/0/options/ ??

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

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec_unix.go#newc...
src/pkg/os/exec_unix.go:25: var status syscall.WaitStatus
if p.Pid == -1 {
    return nil, EINVAL
}

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec_unix.go#newc...
src/pkg/os/exec_unix.go:42: // Release resources associated with the Process,
This sentence is missing a verb (Release is a noun).

// Release releases any resources associated with the Process.

can delete part about Error.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec_unix.go#newc...
src/pkg/os/exec_unix.go:45: // NOOP for unix.
p.Pid = -1
return nil

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec_unix.go#newc...
src/pkg/os/exec_unix.go:52: // If the process cannot be found, an error is
returned.
delete

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

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec_windows.go#n...
src/pkg/os/exec_windows.go:28: return ErrorString("os: Process is already
released")
this is fine, but so is return EINVAL.
(same thing Close on a closed file does.)

http://codereview.appspot.com/4029053/diff/16001/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4029053/diff/16001/src/pkg/syscall/exec_unix.go...
src/pkg/syscall/exec_unix.go:314: // Provide interface to ForkExec to os
package.
// StartProcess wraps ForkExec for package os.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4029053/diff/16001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:677: Notused int32
can delete this line

[rog, 2011-02-03 14:42:12]

On 3 February 2011 01:06,  <alex.brainman@gmail.com> wrote:
> On 2011/02/02 14:35:06, rog wrote:
>>
>> how limited are handles as a resource in windows?
>
> I don't know.
>
>> if there are no strict limits, maybe just closing
>> the handle in the finalizer might be sufficient.
>
> I'm sure, it is possible, but why hide it, it is just one extra line of
> code. Plus I like to have control over resources I hold. I see
> os.Process api to be similar to os.File - you open it, you work with it,
> you tell os that you finished with it.

at the least you should add a finalizer to call Release, as
File does.

[brainman, 2011-02-04 03:11:41]

Hello mattn, r, niemeyer, rog, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-02-04 03:11:58]

Thank you for review.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec.go
File src/pkg/os/exec.go (right):

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec.go#newcode11
src/pkg/os/exec.go:11: // Process stores the information about a process created
by StartProcess.
On 2011/02/03 03:31:15, rsc wrote:
> I wonder if this should be per-OS.
> Fine for now.

Done.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec.go#newcode23
src/pkg/os/exec.go:23: // StartProcess returns an error if the process cannot be
created.
On 2011/02/03 03:31:15, rsc wrote:
> Can delete this (should go without saying).
> I realize that it was in the original; that was my fault.
> 

Done.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec.go#newcode77
src/pkg/os/exec.go:77: // Wait is a shortcut for p:=os.FindProcess(); p.Wait();
p.Release();
On 2011/02/03 03:31:15, rsc wrote:
> // Wait is equivalent to calling FindProcess and then Wait
> // and Release on the result.

Done.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec.go#newcode84
src/pkg/os/exec.go:84: return p.Wait(0)
On 2011/02/03 03:31:15, rsc wrote:
> s/0/options/ ??

Nothing will sneak past you <g>! My mistake. Fixed.

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

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec_unix.go#newc...
src/pkg/os/exec_unix.go:25: var status syscall.WaitStatus
On 2011/02/03 03:31:15, rsc wrote:
> if p.Pid == -1 {
>     return nil, EINVAL
> }

Done.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec_unix.go#newc...
src/pkg/os/exec_unix.go:42: // Release resources associated with the Process,
On 2011/02/03 03:31:15, rsc wrote:
> This sentence is missing a verb (Release is a noun).
> 
> // Release releases any resources associated with the Process.
> 
> can delete part about Error.
> 

Done.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec_unix.go#newc...
src/pkg/os/exec_unix.go:45: // NOOP for unix.
On 2011/02/03 03:31:15, rsc wrote:
> p.Pid = -1
> return nil

Done.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec_unix.go#newc...
src/pkg/os/exec_unix.go:52: // If the process cannot be found, an error is
returned.
On 2011/02/03 03:31:15, rsc wrote:
> delete

Done.

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

http://codereview.appspot.com/4029053/diff/16001/src/pkg/os/exec_windows.go#n...
src/pkg/os/exec_windows.go:28: return ErrorString("os: Process is already
released")
On 2011/02/03 03:31:15, rsc wrote:
> this is fine, but so is return EINVAL.
> (same thing Close on a closed file does.)

Done.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4029053/diff/16001/src/pkg/syscall/exec_unix.go...
src/pkg/syscall/exec_unix.go:314: // Provide interface to ForkExec to os
package.
On 2011/02/03 03:31:15, rsc wrote:
> // StartProcess wraps ForkExec for package os.

Done.

http://codereview.appspot.com/4029053/diff/16001/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4029053/diff/16001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:677: Notused int32
On 2011/02/03 03:31:15, rsc wrote:
> can delete this line

Done.

[brainman, 2011-02-04 03:13:23]

On 2011/02/03 14:42:12, rog wrote:
> at the least you should add a finalizer to call Release, as
> File does.

Sounds reasonable. Done.

Alex

[brainman, 2011-02-04 03:41:33]

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

os: implement new Process api

Fixes issue 1004.
Fixes issue 1460.

R=mattn, r, niemeyer, rog, rsc
CC=golang-dev
http://codereview.appspot.com/4029053