code review 5707052: os: release process handle at the end of windows (*Proc...

os: release process handle at the end of windows (*Process).Wait

Fixes issue 3154.

[brainman, 2012-02-28 23:44:00]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-02-28 23:52:04]

Thanks.  Btw, this is issue 3154.

Are we going to further document the relation between Wait and Release?
 Should Release be documented to say that once it's release, it can no
longer be waited on?  (and enforce that?)


On Tue, Feb 28, 2012 at 3:44 PM, <alex.brainman@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> os: release process handle at the end of windows (*Process).Wait
>
> Please review this at
http://codereview.appspot.com/**5707052/<http://codereview.appspot.com/5707052/>
>
> Affected files:
>  M src/cmd/cgo/util.go
>  M src/cmd/godoc/main.go
>  M src/pkg/net/http/triv.go
>  M src/pkg/os/exec_windows.go
>  M src/pkg/os/os_test.go
>
>
> Index: src/cmd/cgo/util.go
> ==============================**==============================**=======
> --- a/src/cmd/cgo/util.go
> +++ b/src/cmd/cgo/util.go
> @@ -36,7 +36,6 @@
>        if err != nil {
>                fatalf("%s", err)
>        }
> -       defer p.Release()
>        r0.Close()
>        w1.Close()
>        w2.Close()
> Index: src/cmd/godoc/main.go
> ==============================**==============================**=======
> --- a/src/cmd/godoc/main.go
> +++ b/src/cmd/godoc/main.go
> @@ -99,7 +99,6 @@
>                log.Printf("os.StartProcess(%**q): %v", bin, err)
>                return 2
>        }
> -       defer p.Release()
>
>        var buf bytes.Buffer
>        io.Copy(&buf, r)
> Index: src/pkg/net/http/triv.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/triv.go
> +++ b/src/pkg/net/http/triv.go
> @@ -108,7 +108,6 @@
>                fmt.Fprintf(rw, "fork/exec: %s\n", err)
>                return
>        }
> -       defer p.Release()
>        io.Copy(rw, r)
>        wait, err := p.Wait(0)
>        if err != nil {
> Index: src/pkg/os/exec_windows.go
> ==============================**==============================**=======
> --- a/src/pkg/os/exec_windows.go
> +++ b/src/pkg/os/exec_windows.go
> @@ -14,6 +14,7 @@
>
>  // Wait waits for the Process to exit or stop, and then returns a
>  // ProcessState describing its status and an error, if any.
> +// Wait releases any resources associated with the Process.
>  func (p *Process) Wait() (ps *ProcessState, err error) {
>        s, e := syscall.WaitForSingleObject(**syscall.Handle(p.handle),
> syscall.INFINITE)
>        switch s {
> @@ -30,6 +31,7 @@
>                return nil, NewSyscallError("**GetExitCodeProcess", e)
>        }
>        p.done = true
> +       defer p.Release()
>        return &ProcessState{p.Pid, syscall.WaitStatus{Status: s, ExitCode:
> ec}, new(syscall.Rusage)}, nil
>  }
>
> Index: src/pkg/os/os_test.go
> ==============================**==============================**=======
> --- a/src/pkg/os/os_test.go
> +++ b/src/pkg/os/os_test.go
> @@ -530,7 +530,6 @@
>        if err != nil {
>                t.Fatalf("StartProcess: %v", err)
>        }
> -       defer p.Release()
>        w.Close()
>
>        var b bytes.Buffer
> @@ -848,7 +847,6 @@
>        if err != nil {
>                t.Fatal(err)
>        }
> -       defer p.Release()
>        w.Close()
>
>        var b bytes.Buffer
>
>
>

[brainman, 2012-02-29 00:07:45]

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

Please take another look.

[brainman, 2012-02-29 00:07:55]

On 2012/02/28 23:52:04, bradfitz wrote:
> ...  Btw, this is issue 3154.

Done.

> Are we going to further document the relation between Wait and Release?

I did change Wait comment. I think it is enough. If you want more, please tell
me what to say.

>  Should Release be documented to say that once it's release, it can no
> longer be waited on?

Added more to Release comment.

> ...  (and enforce that?)

It is enforces as is - Release sets p.handle to syscall.InvalidHandle. Waiting
on syscall.InvalidHandle will fail. It is like using os.File after you run Close
on it.

Alex

[bradfitz, 2012-02-29 00:12:17]

On Tue, Feb 28, 2012 at 4:07 PM, <alex.brainman@gmail.com> wrote:

> On 2012/02/28 23:52:04, bradfitz wrote:
>
>> ...  Btw, this is issue 3154.
>>
>
> Done.
>
>
>  Are we going to further document the relation between Wait and
>>
> Release?
>
> I did change Wait comment. I think it is enough. If you want more,
> please tell me what to say.
>
>
>   Should Release be documented to say that once it's release, it can no
>> longer be waited on?
>>
>
> Added more to Release comment.
>
>  ...  (and enforce that?)
>>
>
> It is enforces as is - Release sets p.handle to syscall.InvalidHandle.
> Waiting on syscall.InvalidHandle will fail. It is like using os.File
> after you run Close on it.
>
>
Oh, and I see the same in exec_unix.go.  Okay, I'm happy enough.

[bradfitz, 2012-02-29 00:13:54]

LGTM but wait for others.

Also, should the Release docs be mirrored in exec_unix.go?  I think the
public golang.org will only show linux docs, anyway.  I don't know what our
general policy on this sort of doc sync issue is.


On Tue, Feb 28, 2012 at 4:12 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

>
>
> On Tue, Feb 28, 2012 at 4:07 PM, <alex.brainman@gmail.com> wrote:
>
>> On 2012/02/28 23:52:04, bradfitz wrote:
>>
>>> ...  Btw, this is issue 3154.
>>>
>>
>> Done.
>>
>>
>>  Are we going to further document the relation between Wait and
>>>
>> Release?
>>
>> I did change Wait comment. I think it is enough. If you want more,
>> please tell me what to say.
>>
>>
>>   Should Release be documented to say that once it's release, it can no
>>> longer be waited on?
>>>
>>
>> Added more to Release comment.
>>
>>  ...  (and enforce that?)
>>>
>>
>> It is enforces as is - Release sets p.handle to syscall.InvalidHandle.
>> Waiting on syscall.InvalidHandle will fail. It is like using os.File
>> after you run Close on it.
>>
>>
> Oh, and I see the same in exec_unix.go.  Okay, I'm happy enough.
>
>
>

[brainman, 2012-02-29 00:19:03]

On 2012/02/29 00:13:54, bradfitz wrote:
> 
> Also, should the Release docs be mirrored in exec_unix.go? ...

And exec_plan9 ... But I am not sure that it should. The way Russ wants things
(Release is called by Wait automatically), I feel he does not want Release to be
advertised much. But I will wait for him to say.

> ... I think the
> public http://golang.org will only show linux docs, anyway.  I don't know what
our
> general policy on this sort of doc sync issue is.

I can move these docos into doc.go - that is not the problem. It just we need to
decide if we want same comment for all os, and, if yes, for which functions.

Alex

[rsc, 2012-02-29 15:42:42]

Thanks for cleaning this up.  Much improved.

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

http://codereview.appspot.com/5707052/diff/5001/src/pkg/os/exec_windows.go#ne...
src/pkg/os/exec_windows.go:52: // rendering it unusable in the future.
add
// Release only needs to be called if Wait is not.

also add the same to the comments on Release for other systems.

[brainman, 2012-03-01 03:23:23]

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

Please take another look.

[brainman, 2012-03-01 03:23:41]

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

http://codereview.appspot.com/5707052/diff/5001/src/pkg/os/exec_windows.go#ne...
src/pkg/os/exec_windows.go:52: // rendering it unusable in the future.
On 2012/02/29 15:42:42, rsc wrote:
> add
> // Release only needs to be called if Wait is not.
> 
> also add the same to the comments on Release for other systems.

Done.

[rsc, 2012-03-01 06:24:15]

LGTM

[brainman, 2012-03-01 06:36:42]

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

os: release process handle at the end of windows (*Process).Wait

Fixes issue 3154.

R=golang-dev, bradfitz, rsc
CC=golang-dev
http://codereview.appspot.com/5707052