Issue 5699081: code review 5699081: os/exec: release completed process handle (Closed)
Descriptionos/exec: release completed process handle
Patch Set 1 #Patch Set 2 : diff -r cc4ffa82948b https://go.googlecode.com/hg/ #Patch Set 3 : diff -r cc4ffa82948b https://go.googlecode.com/hg/ #MessagesTotal messages: 13
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
I don't like this CL (yet). It doesn't have a test, and I can't understand what os.Process.Release actually does. Its docs are quite lacking. Why doesn't os.Process.Wait call Release automatically? If I call Release, what later becomes valid or invalid? Can I Wait after Release? Or do I have to Release after Wait? On Mon, Feb 27, 2012 at 12:57 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/exec: release completed process handle > > Please review this at http://codereview.appspot.com/**5699081/<http://codereview.appspot.com/5699081/> > > Affected files: > M src/pkg/os/exec/exec.go > > > Index: src/pkg/os/exec/exec.go > ==============================**==============================**======= > --- a/src/pkg/os/exec/exec.go > +++ b/src/pkg/os/exec/exec.go > @@ -307,7 +307,10 @@ > > if err != nil { > return err > - } else if !state.Success() { > + } > + defer c.Process.Release() > + > + if !state.Success() { > return &ExitError{state} > } > > > >
Sign in to reply to this message.
On 2012/02/27 02:10:51, bradfitz wrote: > > It doesn't have a test, ... I can't think of a good way to test this. > ... and I can't understand what os.Process.Release > actually does. ... os.Process struct holds process handle on windows. It needs to be closed sooner or later. os.Process.Release releases that handle. os.Process.Release is called anyway: rex os # grep -n SetFinal * exec.go:21: runtime.SetFinalizer(p, (*Process).Release) but rsc suggested (http://code.google.com/p/go/issues/detail?id=2866#c12) we caller it as soon as we can, rather then later. > ... Its docs are quite lacking. ... func (p *Process) Release() error Release releases any resources associated with the Process. seems enough to me. > ... Why doesn't os.Process.Wait > call Release automatically? Perhaps it can. But I would not like to commit to it at this stage. Perhaps we would want to be able to collect stats for process that finished. Or some other functionality needs to be implemented. For example, I find this in exec_windows.go: // UserTime returns the user CPU time of the exited process and its children. // For now, it is always reported as 0 on Windows. func (p *ProcessState) UserTime() time.Duration { return 0 } > ... If I call Release, what later becomes valid or > invalid? Process handle is closed. So you can't use it anymore. > ... Can I Wait after Release? You can't Wait after you Released process handle. > ... Or do I have to Release after Wait? You must close it like any handle on Windows. If you don't close it, it will get closed once you process goes. Open handles means some system resource are occupied, it means other processes can't use them. Alex
Sign in to reply to this message.
On Mon, Feb 27, 2012 at 1:53 PM, <alex.brainman@gmail.com> wrote: > On 2012/02/27 02:10:51, bradfitz wrote: > > It doesn't have a test, ... >> > > I can't think of a good way to test this. > Can you ask Windows how many open handles a process has? Run a bunch of things and verify that number hasn't increased a bunch. > ... and I can't understand what os.Process.Release >> actually does. ... >> > > os.Process struct holds process handle on windows. It needs to be closed > sooner or later. os.Process.Release releases that handle. > os.Process.Release is called anyway: > > rex os # grep -n SetFinal * > exec.go:21: runtime.SetFinalizer(p, (*Process).Release) > I can read code. I was talking about the godoc. > but rsc suggested > (http://code.google.com/p/go/**issues/detail?id=2866#c12<http://code.google.co...) > we caller it as > soon as we can, rather then later. > > ... Its docs are quite lacking. ... >> > > func (p *Process) Release() error > Release releases any resources associated with the Process. > > seems enough to me. > It doesn't say anything about when I'm supposed to call it, though. I want to know the difference between Wait() and Release(), and not in this email. I want it in the docs.
Sign in to reply to this message.
On 2012/02/27 03:23:53, bradfitz wrote:
>
> Can you ask Windows how many open handles a process has? ...
I do not know. I will investigate.
> >
> > func (p *Process) Release() error
> > Release releases any resources associated with the Process.
> >
> > seems enough to me.
> >
>
> It doesn't say anything about when I'm supposed to call it, though. I want
> to know the difference between Wait() and Release(), and not in this email.
> I want it in the docs.
This is from godoc:
func (p *Process) Release() error
Release releases any resources associated with the Process.
func (p *Process) Wait() (ps *ProcessState, err error)
Wait waits for the Process to exit or stop, and then returns a
ProcessState describing its status and an error, if any.
What else should I say on top of that? You should call Wait, if you want to know
when process is finished. You should call Release, when you finished with
Process - you have no plans to interrogate it anymore. It is similar to
func (f *File) Close() error
Close closes the File, rendering it unusable for I/O. It returns an
error, if any.
Should I add "... rendering it unusable for ..." bit?
Alex
Sign in to reply to this message.
os.Process.Wait should release automatically. It would be even better if then Release was no longer a public method on os.Process. People outside Windows do not think about Release as a required operation; if we don't just get it right automatically, we will certainly have handle leaks. Russ
Sign in to reply to this message.
+1 on removing Release from the public API. On Mon, Feb 27, 2012 at 7:43 AM, Russ Cox <rsc@golang.org> wrote: > os.Process.Wait should release automatically. > It would be even better if then Release was no > longer a public method on os.Process. > People outside Windows do not think about > Release as a required operation; if we don't just > get it right automatically, we will certainly have > handle leaks. > > Russ >
Sign in to reply to this message.
On 2012/02/27 15:43:28, rsc wrote: > os.Process.Wait should release automatically. > It would be even better if then Release was no > longer a public method on os.Process. I am nearly convinced <g>. But one question. If I create a process with os.StartProcess and do not want to wait for it to finish, how do I close handle that I hold? Alex
Sign in to reply to this message.
On Tue, Feb 28, 2012 at 11:09 AM, <alex.brainman@gmail.com> wrote: > On 2012/02/27 15:43:28, rsc wrote: > >> os.Process.Wait should release automatically. >> It would be even better if then Release was no >> longer a public method on os.Process. >> > > I am nearly convinced <g>. But one question. > > If I create a process with os.StartProcess and do not want to wait for > it to finish, how do I close handle that I hold? > go process.Wait() Or, are you saying that Windows lets you close the handle while a process is still running? If so, that can't be much saved memory compared to the memory of the process that's running. So my answer is the same: go process.Wait()
Sign in to reply to this message.
On 2012/02/28 00:13:14, bradfitz wrote: > > > > If I create a process with os.StartProcess and do not want to wait for > > it to finish, how do I close handle that I hold? > > > > go process.Wait() That, certainly, wont do. I do not want to wait for process to finish- it could be running for a while, maybe much longer then my own process. I just want to close process handle I am holding to tell OS that it can release any resources (locks and such) associated to my access of this running process. These could be important when OS gets to make decisions about whether to allow to do things. > Or, are you saying that Windows lets you close the handle while a process > is still running? Absolutely. It is like a file handle - you close file handle, but the file remains. > ... If so, that can't be much saved memory compared to the > memory of the process that's running. ... Memory is not my concern. It is other things (things I do not know about) that bother me. Alex
Sign in to reply to this message.
On Mon, Feb 27, 2012 at 19:09, <alex.brainman@gmail.com> wrote: > If I create a process with os.StartProcess and do not want to wait for > it to finish, how do I close handle that I hold? I don't mind leaving Release if the semantics are that you either Wait or Release but never both. Russ
Sign in to reply to this message.
On 2012/02/28 16:51:11, rsc wrote: > > I don't mind leaving Release if the semantics are that you either Wait > or Release but never both. > OK. http://codereview.appspot.com/5707052/ Alex
Sign in to reply to this message.
|
