Issue 6610066: code review 6610066: os: Added documentation to the Exit function
Descriptionos: Added documentation to the Exit function
Added documentation for the Exit function indicating that when calling Exit,
any defer statements that have been called never actually get executed.
Fixes issue 4101.
Patch Set 1 #Patch Set 2 : diff -r 0e7dab91ba8e https://code.google.com/p/go/ #Patch Set 3 : diff -r 1571ae601243 https://code.google.com/p/go/ #Patch Set 4 : diff -r 1571ae601243 https://code.google.com/p/go/ #Patch Set 5 : diff -r 1571ae601243 https://code.google.com/p/go/ #
Total comments: 1
MessagesTotal messages: 24
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
I'm not sure whether the behavior should be documented or changed.
Sign in to reply to this message.
According to the Go Language specification: "A 'defer' statement invokes a function whose execution is deferred to the *moment the surrounding function returns*." (emphasis mine). Since calling the Exit by definition (at least the "standard" definition I understand) immediately terminates the current running process, the current function would have no way to return, i.e. the current behavior is correct. That said, it certainly feels like the current behavior defeats the spirit of the "defer" statement. Perhaps the spec on defer should be made a little looser so as to accommodate cases such as this? On Sun, Oct 7, 2012 at 7:22 PM, Rob Pike <r@golang.org> wrote: > I'm not sure whether the behavior should be documented or changed. > -- Peace, Love, and Source Code -Kurtis
Sign in to reply to this message.
Erroneously sent to OP only, FWDing now. ---------- Forwarded message ---------- From: Jan Mercl <0xjnml@gmail.com> Date: Mon, Oct 8, 2012 at 10:07 AM Subject: Re: [golang-dev] code review 6610066: os: Added documentation to the Exit function (issue 6610066) To: Kurtis Nusbaum <klnusbaum@gmail.com> On Mon, Oct 8, 2012 at 3:44 AM, Kurtis Nusbaum <klnusbaum@gmail.com> wrote: I would say Looks Good To Me if I could. 1) The behaviour of os.Exit is settled for years (omg, time passes so fast!). 2) A buggy code in a defer can cause a non-exitable, only kill-able Go binary. Bad for users, bad for Go reputation. 3) What other mechanism will be left for legit cases where the program really wants to exit NQA? -j
Sign in to reply to this message.
I was trying to imagine how far one takes this, if deferred functions need to be invoked all the way up a chain of function calls. Not a pretty sight. Lucio. On 10/8/12, Jan Mercl <0xjnml@gmail.com> wrote: > Erroneously sent to OP only, FWDing now. > > > ---------- Forwarded message ---------- > From: Jan Mercl <0xjnml@gmail.com> > Date: Mon, Oct 8, 2012 at 10:07 AM > Subject: Re: [golang-dev] code review 6610066: os: Added documentation > to the Exit function (issue 6610066) > To: Kurtis Nusbaum <klnusbaum@gmail.com> > > > On Mon, Oct 8, 2012 at 3:44 AM, Kurtis Nusbaum <klnusbaum@gmail.com> wrote: > > I would say Looks Good To Me if I could. > > 1) The behaviour of os.Exit is settled for years (omg, time passes so > fast!). > 2) A buggy code in a defer can cause a non-exitable, only kill-able Go > binary. Bad for users, bad for Go reputation. > 3) What other mechanism will be left for legit cases where the program > really wants to exit NQA? > > -j >
Sign in to reply to this message.
On Sun, Oct 7, 2012 at 8:22 PM, Rob Pike <r@golang.org> wrote: > I'm not sure whether the behavior should be documented or changed. I don't think changing the behavior makes sense here. The parallel is with runtime.Goexit, but runtime.Goexit runs the deferred calls of the one goroutine that it is terminating, which importantly is the goroutine that called Goexit, so it is in one of a limited number of states. os.Exit could really only run the defers from the current goroutine - interrupting other goroutines and forcibly running their defers at unspecified execution points would not work well - so you'd have to start thinking about which goroutine calls Exit. You'd also have to introduce a version of Exit that doesn't do this, for use by things that really just want to die, like maybe log.Fatal. This would also reintroduce all the problems C has with atexit. Russ
Sign in to reply to this message.
Russ, I actually think you're reasoning is pretty sound on this one. I fully agree. So can we just include the documentation I wrote as is then? On Mon, Oct 8, 2012 at 9:50 AM, Russ Cox <rsc@golang.org> wrote: > On Sun, Oct 7, 2012 at 8:22 PM, Rob Pike <r@golang.org> wrote: > > I'm not sure whether the behavior should be documented or changed. > > I don't think changing the behavior makes sense here. The parallel is > with runtime.Goexit, but runtime.Goexit runs the deferred calls of the > one goroutine that it is terminating, which importantly is the > goroutine that called Goexit, so it is in one of a limited number of > states. os.Exit could really only run the defers from the current > goroutine - interrupting other goroutines and forcibly running their > defers at unspecified execution points would not work well - so you'd > have to start thinking about which goroutine calls Exit. You'd also > have to introduce a version of Exit that doesn't do this, for use by > things that really just want to die, like maybe log.Fatal. This would > also reintroduce all the problems C has with atexit. > > Russ > -- Peace, Love, and Source Code -Kurtis
Sign in to reply to this message.
For what it's worth: LGTM I agree w/ the various comments that the behavior of Exit cannot reasonably be changed. If we wanted an exit that behaves differently (say calls that deferred functions) it should be another function (os.GoExit ?). That said, for the reasons also outlined before (goroutines), I don't know what the exact semantics of such a function should be and if it can be reasonably implemented.
Sign in to reply to this message.
gri, I'm not a committer. Can you please run: $hg clpatch 6610066 $hg submit 6610066 On Mon, Oct 8, 2012 at 1:38 PM, <gri@golang.org> wrote: > For what it's worth: > > LGTM > > I agree w/ the various comments that the behavior of Exit cannot > reasonably be changed. > > If we wanted an exit that behaves differently (say calls that deferred > functions) it should be another function (os.GoExit ?). That said, for > the reasons also outlined before (goroutines), I don't know what the > exact semantics of such a function should be and if it can be reasonably > implemented. > > https://codereview.appspot.**com/6610066/<https://codereview.appspot.com/6610... > -- Peace, Love, and Source Code -Kurtis
Sign in to reply to this message.
I was aware of that. But I am leaving this for feedback from others (r, rsc, iant). - gri On Mon, Oct 8, 2012 at 12:29 PM, Kurtis Nusbaum <klnusbaum@gmail.com> wrote: > gri, > I'm not a committer. Can you please run: > > $hg clpatch 6610066 > $hg submit 6610066 > > > On Mon, Oct 8, 2012 at 1:38 PM, <gri@golang.org> wrote: > >> For what it's worth: >> >> LGTM >> >> I agree w/ the various comments that the behavior of Exit cannot >> reasonably be changed. >> >> If we wanted an exit that behaves differently (say calls that deferred >> functions) it should be another function (os.GoExit ?). That said, for >> the reasons also outlined before (goroutines), I don't know what the >> exact semantics of such a function should be and if it can be reasonably >> implemented. >> >> https://codereview.appspot.**com/6610066/<https://codereview.appspot.com/6610... >> > > > > -- > Peace, Love, and Source Code > -Kurtis > >
Sign in to reply to this message.
Ah ok. Sorry, didn't realize that. I'll wait then :) On Mon, Oct 8, 2012 at 2:31 PM, Robert Griesemer <gri@golang.org> wrote: > I was aware of that. But I am leaving this for feedback from others (r, > rsc, iant). > - gri > > > On Mon, Oct 8, 2012 at 12:29 PM, Kurtis Nusbaum <klnusbaum@gmail.com>wrote: > >> gri, >> I'm not a committer. Can you please run: >> >> $hg clpatch 6610066 >> $hg submit 6610066 >> >> >> On Mon, Oct 8, 2012 at 1:38 PM, <gri@golang.org> wrote: >> >>> For what it's worth: >>> >>> LGTM >>> >>> I agree w/ the various comments that the behavior of Exit cannot >>> reasonably be changed. >>> >>> If we wanted an exit that behaves differently (say calls that deferred >>> functions) it should be another function (os.GoExit ?). That said, for >>> the reasons also outlined before (goroutines), I don't know what the >>> exact semantics of such a function should be and if it can be reasonably >>> implemented. >>> >>> https://codereview.appspot.**com/6610066/<https://codereview.appspot.com/6610... >>> >> >> >> >> -- >> Peace, Love, and Source Code >> -Kurtis >> >> > -- Peace, Love, and Source Code -Kurtis
Sign in to reply to this message.
I'd be happier if the spec was clearer. It says the deferred function runs "when the function returns" but it's not that simple. The text should be cleared up so that the behavior we want for os.Exit falls out, something like "when the the function returns normally or terminates due to a panicking function". Not great wording but you get the idea. -rob
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, r@golang.org, 0xjnml@gmail.com, lucio.dere@gmail.com, rsc@golang.org, gri@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, r@golang.org, 0xjnml@gmail.com, lucio.dere@gmail.com, rsc@golang.org, gri@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Still waiting for a response from others about my request to tweak the spec. -rob
Sign in to reply to this message.
Any response on the spec tweak? On Tue, Oct 9, 2012 at 6:40 PM, Rob Pike <r@golang.org> wrote: > Still waiting for a response from others about my request to tweak the > spec. > > -rob > -- Peace, Love, and Source Code -Kurtis
Sign in to reply to this message.
I will look into this tomorrow. - gri On Mon, Oct 22, 2012 at 8:53 PM, Kurtis Nusbaum <klnusbaum@gmail.com> wrote: > Any response on the spec tweak? > > > On Tue, Oct 9, 2012 at 6:40 PM, Rob Pike <r@golang.org> wrote: > >> Still waiting for a response from others about my request to tweak the >> spec. >> >> -rob >> > > > > -- > Peace, Love, and Source Code > -Kurtis > >
Sign in to reply to this message.
syscall.Exec doesn't return either, and whether to call that or Exit is a deliberate choice of the programmer. """ A "defer" statement invokes a function whose execution is deferred to the moment the surrounding function returns. .... Some functions, like those calling syscall.Exec or syscall.Exit, may never return, and if so, any deferred functions pending at the time of the call to them will not be executed. """ Then the documentation of syscall.Exec, syscall.Exit and any callers in the standard libraries can be updated to say that if they don't return, the pending deferred calls will not run. /L On Tue, Oct 23, 2012 at 6:04 AM, Robert Griesemer <gri@golang.org> wrote: > I will look into this tomorrow. > - gri > > > On Mon, Oct 22, 2012 at 8:53 PM, Kurtis Nusbaum <klnusbaum@gmail.com> wrote: >> >> Any response on the spec tweak? >> >> >> On Tue, Oct 9, 2012 at 6:40 PM, Rob Pike <r@golang.org> wrote: >>> >>> Still waiting for a response from others about my request to tweak the >>> spec. >>> >>> -rob >> >> >> >> >> -- >> Peace, Love, and Source Code >> -Kurtis >> >
Sign in to reply to this message.
See: http://codereview.appspot.com/6736071 - gri On Mon, Oct 22, 2012 at 9:04 PM, Robert Griesemer <gri@golang.org> wrote: > I will look into this tomorrow. > - gri > > > On Mon, Oct 22, 2012 at 8:53 PM, Kurtis Nusbaum <klnusbaum@gmail.com>wrote: > >> Any response on the spec tweak? >> >> >> On Tue, Oct 9, 2012 at 6:40 PM, Rob Pike <r@golang.org> wrote: >> >>> Still waiting for a response from others about my request to tweak the >>> spec. >>> >>> -rob >>> >> >> >> >> -- >> Peace, Love, and Source Code >> -Kurtis >> >> >
Sign in to reply to this message.
So what should I do with 6610066? Should I just delete this patch? On Tue, Oct 23, 2012 at 5:41 PM, Robert Griesemer <gri@golang.org> wrote: > See: http://codereview.appspot.com/6736071 > > - gri > > > On Mon, Oct 22, 2012 at 9:04 PM, Robert Griesemer <gri@golang.org> wrote: > >> I will look into this tomorrow. >> - gri >> >> >> On Mon, Oct 22, 2012 at 8:53 PM, Kurtis Nusbaum <klnusbaum@gmail.com>wrote: >> >>> Any response on the spec tweak? >>> >>> >>> On Tue, Oct 9, 2012 at 6:40 PM, Rob Pike <r@golang.org> wrote: >>> >>>> Still waiting for a response from others about my request to tweak the >>>> spec. >>>> >>>> -rob >>>> >>> >>> >>> >>> -- >>> Peace, Love, and Source Code >>> -Kurtis >>> >>> >> > -- Peace, Love, and Source Code -Kurtis
Sign in to reply to this message.
http://codereview.appspot.com/6610066/diff/17001/src/pkg/os/proc.go File src/pkg/os/proc.go (right): http://codereview.appspot.com/6610066/diff/17001/src/pkg/os/proc.go#newcode37 src/pkg/os/proc.go:37: // function is reached. now the spec has the necessary covering prose, we can write this. but let's make it shorter and to the point. // Exit causes the program to exit with the given status code. // By convention, code zero indicates success, non-zero an error. // The program terminates immediately; deferred functions are // not run.
Sign in to reply to this message.
On 2012/11/02 00:38:17, r wrote: > http://codereview.appspot.com/6610066/diff/17001/src/pkg/os/proc.go > File src/pkg/os/proc.go (right): > > http://codereview.appspot.com/6610066/diff/17001/src/pkg/os/proc.go#newcode37 > src/pkg/os/proc.go:37: // function is reached. > now the spec has the necessary covering prose, we can write this. but let's make > it shorter and to the point. > > // Exit causes the program to exit with the given status code. > // By convention, code zero indicates success, non-zero an error. > // The program terminates immediately; deferred functions are > // not run. Should somebody be reminded of this? I'm lucky to have only a few issues to look into, this one goes back a long way! Lucio.
Sign in to reply to this message.
Replacing golang-dev with golang-codereviews.
Sign in to reply to this message.
|
