* using trap function to cleanup and exit? @ 2022-04-10 15:46 Greg Klanderman 2022-04-10 15:57 ` Peter Stephenson 0 siblings, 1 reply; 17+ messages in thread From: Greg Klanderman @ 2022-04-10 15:46 UTC (permalink / raw) To: Zsh list Hi, I've been trying to ensure some cleanup is done when a script I'm writing exits, whether normally, with an error, or when killed (other than with -9, of course). I had thought a try/always block would handle signals as well, but it appears not. So I've been trying to use either trap functions or the trap builtin (list traps) with no success. [zsh below is debian/5.8.1-1 (Debian testing)] Here's a simple test script (the sleep has been substituted for an external command that waits on a condition and outputs something; my actual use case also has that within a loop): <script> #!/bin/zsh -f cleanup () { echo "cleanup" } do_something () { echo "in do_something" local foo read -u 0 foo echo "done do_something" } TRAPTERM () { echo "in TRAPTERM" cleanup TERM exit $(( 128 + $1 )) } foo () { echo "in foo" sleep 1234567 | do_something } foo </script> When run, it immediately prints: | in foo | in do_something then when hit with SIGTERM: | in TRAPTERM | cleanup however, the process and its child (sleep) remain running. I expected both to exit. Additionally, when I hit it with some other signal (SIGINT), the script does get killed, however, the child sleep process remains. Is that expected? I tried TRAPEXIT as well, both at top level and within the function foo, but that seems not to handle signals. How can I ensure that a script and non-disowned children are killed when a signal is received, after doing some cleanup? Is there a way to do that for all appropriate signals, or do I have to explicitly list the desired signals? thank you, Greg ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-10 15:46 using trap function to cleanup and exit? Greg Klanderman @ 2022-04-10 15:57 ` Peter Stephenson 2022-04-10 16:12 ` Greg Klanderman 0 siblings, 1 reply; 17+ messages in thread From: Peter Stephenson @ 2022-04-10 15:57 UTC (permalink / raw) To: zsh-workers On Sun, 2022-04-10 at 11:46 -0400, Greg Klanderman wrote: > TRAPTERM () { > echo "in TRAPTERM" > cleanup TERM > exit $(( 128 + $1 )) > } I haven't gone through this in great detail, so no guarantee this causes everything to spring into life, but just to note that normal service here would be obtained if you turn that "exit" into "return". As described in the Trap Functions in zshmisc, that's specially handled so that the shell knows you want to continue to exit in a similar way to if SIGTERM had been received without the trap function. pws ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-10 15:57 ` Peter Stephenson @ 2022-04-10 16:12 ` Greg Klanderman 2022-04-10 16:30 ` Greg Klanderman 0 siblings, 1 reply; 17+ messages in thread From: Greg Klanderman @ 2022-04-10 16:12 UTC (permalink / raw) To: zsh-workers Hi Peter, I thought I'd changed that last night when I was reading that section of the manual, when I changed the exit value 1 to $(( 128 + $1 )), but apparently not. That does seem to be working better, in that the script itself does exit after running the cleanup, but the child sleep process still remains. I am playing with 'kill -9 -$$' in the trap function, but that seems like an awfully large hammer. thank you, Greg >>>>> On April 10, 2022 Peter Stephenson <p.w.stephenson@ntlworld.com> wrote: > On Sun, 2022-04-10 at 11:46 -0400, Greg Klanderman wrote: >> TRAPTERM () { >> echo "in TRAPTERM" >> cleanup TERM >> exit $(( 128 + $1 )) >> } > I haven't gone through this in great detail, so no guarantee this causes > everything to spring into life, but just to note that normal service > here would be obtained if you turn that "exit" into "return". As > described in the Trap Functions in zshmisc, that's specially handled so > that the shell knows you want to continue to exit in a similar way to if > SIGTERM had been received without the trap function. > pws ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-10 16:12 ` Greg Klanderman @ 2022-04-10 16:30 ` Greg Klanderman 2022-04-10 18:15 ` Lawrence Velázquez 2022-04-10 20:49 ` Bart Schaefer 0 siblings, 2 replies; 17+ messages in thread From: Greg Klanderman @ 2022-04-10 16:30 UTC (permalink / raw) To: zsh-workers Is there an equivalent for exiting when using the trap builtin (list traps)? I don't completely understand the manual: | a return from a list trap causes the surrounding context to return | with the given status I've tried both exit and return in a list trap, and am not seeing the script exit. Also, is exit intentionally disabled from within a trap function? I didn't expect that, even with the special return handling. So, there is no way to exit 0 from a trap, since that is interpreted as the signal having been handled, and wanting to continue executing? With no traps of either type, should the child sleep remain after the script is killed by a signal? thank you, Greg >>>>> On April 10, 2022 Greg Klanderman <gak@klanderman.net> wrote: > Hi Peter, > I thought I'd changed that last night when I was reading that section > of the manual, when I changed the exit value 1 to $(( 128 + $1 )), but > apparently not. > That does seem to be working better, in that the script itself does > exit after running the cleanup, but the child sleep process still > remains. > I am playing with 'kill -9 -$$' in the trap function, but that seems > like an awfully large hammer. > thank you, > Greg >>>>> On April 10, 2022 Peter Stephenson <p.w.stephenson@ntlworld.com> wrote: >> On Sun, 2022-04-10 at 11:46 -0400, Greg Klanderman wrote: >>> TRAPTERM () { >>> echo "in TRAPTERM" >>> cleanup TERM >>> exit $(( 128 + $1 )) >>> } >> I haven't gone through this in great detail, so no guarantee this causes >> everything to spring into life, but just to note that normal service >> here would be obtained if you turn that "exit" into "return". As >> described in the Trap Functions in zshmisc, that's specially handled so >> that the shell knows you want to continue to exit in a similar way to if >> SIGTERM had been received without the trap function. >> pws ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-10 16:30 ` Greg Klanderman @ 2022-04-10 18:15 ` Lawrence Velázquez 2022-04-14 5:13 ` Greg Klanderman 2022-04-10 20:49 ` Bart Schaefer 1 sibling, 1 reply; 17+ messages in thread From: Lawrence Velázquez @ 2022-04-10 18:15 UTC (permalink / raw) To: Greg Klanderman; +Cc: zsh-workers On Sun, Apr 10, 2022, at 12:30 PM, Greg Klanderman wrote: > With no traps of either type, should the child sleep remain after the > script is killed by a signal? Child processes are not automatically terminated if their parent dies. In general you have to handle this yourself (for example, https://mywiki.wooledge.org/SignalTrap#When_is_the_signal_handled.3F). I don't know if zsh has some special sauce to streamline this, as I rarely write scripts that spawn long-running processes. -- vq ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-10 18:15 ` Lawrence Velázquez @ 2022-04-14 5:13 ` Greg Klanderman 0 siblings, 0 replies; 17+ messages in thread From: Greg Klanderman @ 2022-04-14 5:13 UTC (permalink / raw) To: zsh-workers >>>>> On April 10, 2022 Lawrence Velázquez <larryv@zsh.org> wrote: > On Sun, Apr 10, 2022, at 12:30 PM, Greg Klanderman wrote: >> With no traps of either type, should the child sleep remain after the >> script is killed by a signal? > Child processes are not automatically terminated if their parent > dies. In general you have to handle this yourself (for example, > https://mywiki.wooledge.org/SignalTrap#When_is_the_signal_handled.3F). > I don't know if zsh has some special sauce to streamline this, as > I rarely write scripts that spawn long-running processes. Thank you Larry, of course that link is describing bash, not zsh, and it's not immediately clear if they do/should behave the same in this regard. It does say that if an external foreground command is executing in bash, that signals will not be handled until the command terminates. And so the workaround is to background the command, in which case that command will not be killed when the script is killed. This is fine, because you have the ability to capture the PID when you background it, and so can handle killing it if you so desire. The zsh script I posted has a foreground sleep, which does not seem to prevent a signal from being handled as described for bash. But with a foreground command you have no ability to handle killing it when you die, so it would only make sense to me for zsh to do that. I need to study this and what Bart wrote a bunch more, but unfortunately will not be able to do so for at least another week. thank you, Greg ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-10 16:30 ` Greg Klanderman 2022-04-10 18:15 ` Lawrence Velázquez @ 2022-04-10 20:49 ` Bart Schaefer 2022-04-10 20:55 ` Bart Schaefer 2022-04-14 5:57 ` Greg Klanderman 1 sibling, 2 replies; 17+ messages in thread From: Bart Schaefer @ 2022-04-10 20:49 UTC (permalink / raw) To: Greg Klanderman; +Cc: Zsh hackers list On Sun, Apr 10, 2022 at 9:30 AM Greg Klanderman <gak@klanderman.net> wrote: > > I've tried both exit and return in a list trap, and am not seeing the > script exit. The rules for traps and child processes are a bit hard to follow. If a signal trap is set to something other than the default in the parent, then that signal is supposed to be blocked in the child, which means the signal won't be propagated from the parent to the child. The following options also control signaling behavior: MONITOR - off by default in scripts, and when off, causes background jobs to be left running when the shell exits. POSIX_JOBS - off by default in native mode, when on forces MONITOR off in subshells HUP - on by default, and if MONITOR is also set, causes child processes to be sent a SIGHUP when the parent exits TRAPS_ASYNC - off by default, when on the parent handles signals immediately, otherwise foreground children must exit first POSIX_TRAPS - off in native mode, modifies the behavior of the EXIT trap (on in sh/bash/ksh emulations) LOCAL_TRAPS - off in native mode, saves trap state on function entry and restores on function return > Also, is exit intentionally disabled from within a trap function? No; there was a bugfix for a related thing in 5.6.1 and a couple of other exit-from-a-trap tweaks involving loop structures that are pending the next release, but exit from a trap should work (and does in some simple tests I did). Do you have a simple example, and are you sure you're not seeing the effects of some of the above options? > I didn't expect that, even with the special return handling. So, > there is no way to exit 0 from a trap, since that is interpreted as > the signal having been handled, and wanting to continue executing? Again I'm not seeing this. If I exit zero from an INT trap, the exit status of the interrupted script is zero. However, if I exit from an exit trap, the status of the exit trap is ignored and I get the status from before it was called, e.g., killing a script with SIGINT always returns 130 even when TRAPEXIT calls exit with a different value. I'm not sure that's clearly documented anywhere. > With no traps of either type, should the child sleep remain after the > script is killed by a signal? See above RE signal options. Probably yes. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-10 20:49 ` Bart Schaefer @ 2022-04-10 20:55 ` Bart Schaefer 2022-04-14 5:57 ` Greg Klanderman 1 sibling, 0 replies; 17+ messages in thread From: Bart Schaefer @ 2022-04-10 20:55 UTC (permalink / raw) To: Greg Klanderman; +Cc: Zsh hackers list On Sun, Apr 10, 2022 at 1:49 PM Bart Schaefer <schaefer@brasslantern.com> wrote: > > However, if I exit from an > exit trap, the status of the exit trap is ignored and I get the status > from before it was called Clarification: If an exit trap is called from another trap, this is the behavior. If the shell is otherwise exiting normally, the status from the exit trap is preserved. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-10 20:49 ` Bart Schaefer 2022-04-10 20:55 ` Bart Schaefer @ 2022-04-14 5:57 ` Greg Klanderman 2022-04-14 21:29 ` Bart Schaefer 1 sibling, 1 reply; 17+ messages in thread From: Greg Klanderman @ 2022-04-14 5:57 UTC (permalink / raw) To: zsh-workers Hi Bart, thank you for your reply and sorry for the delay getting back to you. I haven't had enough time to fully explore this yet, and won't for at least another week. Some replies inline.. >>>>> On April 10, 2022 Bart Schaefer <schaefer@brasslantern.com> wrote: > On Sun, Apr 10, 2022 at 9:30 AM Greg Klanderman <gak@klanderman.net> wrote: >> >> I've tried both exit and return in a list trap, and am not seeing the >> script exit. > The rules for traps and child processes are a bit hard to follow. If > a signal trap is set to something other than the default in the > parent, then that signal is supposed to be blocked in the child, which > means the signal won't be propagated from the parent to the child. > The following options also control signaling behavior: > MONITOR - off by default in scripts, and when off, causes background > jobs to be left running when the shell exits. Right, I expect background processes to be left running by default. > POSIX_JOBS - off by default in native mode, when on forces MONITOR off > in subshells > HUP - on by default, and if MONITOR is also set, causes child > processes to be sent a SIGHUP when the parent exits > TRAPS_ASYNC - off by default, when on the parent handles signals > immediately, otherwise foreground children must exit first This TRAPS_ASYNC default seems to reflect what the page Larry linked to described for bash. The script I posted is using /bin/zsh -f, so TRAPS_ASYNC should be off, but I do see a trap run immediately, when the script is running <external-program> | <shell-function> at the time the signal is received. Is the <external-program> considered a foreground child? > POSIX_TRAPS - off in native mode, modifies the behavior of the EXIT > trap (on in sh/bash/ksh emulations) > LOCAL_TRAPS - off in native mode, saves trap state on function entry > and restores on function return >> Also, is exit intentionally disabled from within a trap function? > No; there was a bugfix for a related thing in 5.6.1 and a couple of > other exit-from-a-trap tweaks involving loop structures that are > pending the next release, but exit from a trap should work (and does > in some simple tests I did). Do you have a simple example, and are > you sure you're not seeing the effects of some of the above options? See the #!/bin/zsh -f script I posted originally - I think it is fairly simple, and zsh -f should ensure the options are in a known default state. When TRAPTERM uses 'exit' rather than 'return', the script does not exit after receiving SIGTERM. With 'return', the script does exit, but the <external-program> in <external-program> | <shell-function> remains running. Since it was not backgrounded, I'm not sure how to make sense of the expected behavior based on the options you described above. It seems to me like it should be killed, as there was no way to capture its PID in order to arrange for it to be killed at exit. thank you, Greg >> I didn't expect that, even with the special return handling. So, >> there is no way to exit 0 from a trap, since that is interpreted as >> the signal having been handled, and wanting to continue executing? > Again I'm not seeing this. If I exit zero from an INT trap, the exit > status of the interrupted script is zero. However, if I exit from an > exit trap, the status of the exit trap is ignored and I get the status > from before it was called, e.g., killing a script with SIGINT always > returns 130 even when TRAPEXIT calls exit with a different value. I'm > not sure that's clearly documented anywhere. >> With no traps of either type, should the child sleep remain after the >> script is killed by a signal? > See above RE signal options. Probably yes. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-14 5:57 ` Greg Klanderman @ 2022-04-14 21:29 ` Bart Schaefer 2022-04-14 23:35 ` Bart Schaefer 2022-04-15 22:27 ` Bart Schaefer 0 siblings, 2 replies; 17+ messages in thread From: Bart Schaefer @ 2022-04-14 21:29 UTC (permalink / raw) To: Greg Klanderman; +Cc: Zsh hackers list [-- Attachment #1: Type: text/plain, Size: 2041 bytes --] On Wed, Apr 13, 2022 at 10:58 PM Greg Klanderman <gak@klanderman.net> wrote: > > The script I posted is using /bin/zsh -f, so TRAPS_ASYNC should be > off, but I do see a trap run immediately, when the script is running > > <external-program> | <shell-function> > > at the time the signal is received. > > Is the <external-program> considered a foreground child? No, it's in the background; zsh "forks to the left" so that the <shell-function> is running in the current shell. This is the reverse of bash/ksh. (In recent zsh, when emulating sh, both sides of the pipe may be forked.) > When TRAPTERM uses 'exit' rather than 'return', the script does not > exit after receiving SIGTERM. OK, this is indeed a bug, previously discussed in workers/44007 and follow-ups. More on this below. > With 'return', the script does exit, > but the <external-program> in > > <external-program> | <shell-function> > > remains running. This is actually expected, I believe; as noted above, <external-program> is a background job here, and it would normally be expected to exit on SIGPIPE when the <shell-function> exits and closes the connection. It never gets that signal because it never writes anything on the pipe. Regarding workers/44007: This seems ultimately to be a side-effect described by this comment: * We don't exit directly from functions to allow tidying * up, in particular EXIT traps. We still need to perform * the usual interactive tests to see if we can exit at * all, however. In bin_break(), which handles both "return" and "exit", the conditions applied upon "return" are not the same as those applied upon "exit". With the builtin.c hunk of the patch below "make check" reports: Test ./C03traps.ztst was expected to fail, but passed. Was testing: (workers/44007) function execution continues after 'exit' in trap I've therefore included cleaning up the BUGS file and reversing the sense of that test, but would prefer that another set of eyes review the code change. [-- Attachment #2: patch44007.txt --] [-- Type: text/plain, Size: 1541 bytes --] diff --git a/Etc/BUGS b/Etc/BUGS index 5624fb24d..3121fc9fa 100644 --- a/Etc/BUGS +++ b/Etc/BUGS @@ -30,9 +30,6 @@ skipped when STTY=... is set for that command ------------------------------------------------------------------------ 42609: :|: =(hang) ------------------------------------------------------------------------ -44007 - Martijn - exit in trap executes rest of function -See test case in Test/C03traps.ztst. ------------------------------------------------------------------------- 44133 debian #924736 (partial patch in 44134) three setopts following ` #` ------------------------------------------------------------------------ 44850 terminal issues with continuation markers diff --git a/Src/builtin.c b/Src/builtin.c index 8ef678b22..b93466ba5 100644 --- a/Src/builtin.c +++ b/Src/builtin.c @@ -5720,6 +5720,8 @@ bin_break(char *name, char **argv, UNUSED(Options ops), int func) * a bad job. */ if (stopmsg || (zexit(0, ZEXIT_DEFERRED), !stopmsg)) { + if (trap_state) + trap_state = TRAP_STATE_FORCE_RETURN; retflag = 1; breaks = loops; exit_pending = 1; diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst index 6f84e5db2..3bd2958cb 100644 --- a/Test/C03traps.ztst +++ b/Test/C03traps.ztst @@ -901,7 +901,7 @@ F:Must be tested with a top-level script rather than source or function fn trap1 trap2 echo out2 ' --f:(workers/44007) function execution continues after 'exit' in trap +-:(workers/44007) function execution continues after 'exit' in trap >out1 >fn1 >trap1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-14 21:29 ` Bart Schaefer @ 2022-04-14 23:35 ` Bart Schaefer 2022-04-17 17:08 ` Daniel Shahaf 2022-04-15 22:27 ` Bart Schaefer 1 sibling, 1 reply; 17+ messages in thread From: Bart Schaefer @ 2022-04-14 23:35 UTC (permalink / raw) To: Greg Klanderman; +Cc: Zsh hackers list On Thu, Apr 14, 2022 at 2:29 PM Bart Schaefer <schaefer@brasslantern.com> wrote: > > I've therefore included cleaning up the BUGS file and reversing the > sense of that test I suppose I should also have reversed the sense of the test description. diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst index 3bd2958cb..8d1283552 100644 --- a/Test/C03traps.ztst +++ b/Test/C03traps.ztst @@ -901,7 +901,7 @@ F:Must be tested with a top-level script rather than source or function fn trap1 trap2 echo out2 ' --:(workers/44007) function execution continues after 'exit' in trap +-:'exit' in trap causes calling function to return >out1 >fn1 >trap1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-14 23:35 ` Bart Schaefer @ 2022-04-17 17:08 ` Daniel Shahaf 0 siblings, 0 replies; 17+ messages in thread From: Daniel Shahaf @ 2022-04-17 17:08 UTC (permalink / raw) To: Zsh hackers list; +Cc: Greg Klanderman Bart Schaefer wrote on Thu, 14 Apr 2022 23:35 +00:00: > On Thu, Apr 14, 2022 at 2:29 PM Bart Schaefer <schaefer@brasslantern.com> wrote: >> >> I've therefore included cleaning up the BUGS file and reversing the >> sense of that test > > I suppose I should also have reversed the sense of the test description. > > diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst > index 3bd2958cb..8d1283552 100644 > --- a/Test/C03traps.ztst > +++ b/Test/C03traps.ztst > @@ -901,7 +901,7 @@ F:Must be tested with a top-level script rather > than source or function > fn trap1 trap2 > echo out2 > ' > --:(workers/44007) function execution continues after 'exit' in trap > +-:'exit' in trap causes calling function to return Change «-» to «0» (or whatever exit code is appropriate)? When a test is XFail ('f' flag), I like to write its expectations as minimally as possible, in order to make it easy for the test to XPass ("was expected to fail, but passed"); however, once the bug is fixed and the test starts to pass (= without 'f'), expectations can be tightened. Not awake enough to review the C changes right now, sorry. Cheers, Daniel > >out1 > >fn1 > >trap1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-14 21:29 ` Bart Schaefer 2022-04-14 23:35 ` Bart Schaefer @ 2022-04-15 22:27 ` Bart Schaefer 2022-04-19 18:42 ` Peter Stephenson 1 sibling, 1 reply; 17+ messages in thread From: Bart Schaefer @ 2022-04-15 22:27 UTC (permalink / raw) To: Zsh hackers list On Thu, Apr 14, 2022 at 2:29 PM Bart Schaefer <schaefer@brasslantern.com> wrote: > > [I] would prefer that another set of eyes review > the code change. To expand on that a bit ... The BIN_RETURN branch checks that (trap_return == -2) which is a pretty specific number. exec.c says: * This is only active if we are inside a trap, else its value * is irrelevant. It is initialised to -1 for a function trap and * -2 for a non-function trap and if negative is decremented as * we go deeper into functions and incremented as we come back up. * The value is used to decide if an explicit "return" should cause * a return from the caller of the trap; it does this by setting * trap_return to a status (i.e. a non-negative value). My interpretation is that, since we are in an explicit "exit" rather than an explicit "return", we don't really care how trap_return is set; we're going to force the caller to return, period. However, the test for whether to increment trap_return again on the way out is dependent on trap_state == TRAP_STATE_PRIMED, so if we change trap_state we're never going to increment (or decrement, actually) trap_return again. (It may, however, get restored from a stacked value.) I can imagine there being some side-effect of this from using a mix of "return" and "exit" from traps or from functions that might be called by traps, but have no good idea how to exercise such a code path. I guess I'll go ahead and push this and if someone spots a problem we can do a revision. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-15 22:27 ` Bart Schaefer @ 2022-04-19 18:42 ` Peter Stephenson 2022-04-19 18:55 ` Peter Stephenson 0 siblings, 1 reply; 17+ messages in thread From: Peter Stephenson @ 2022-04-19 18:42 UTC (permalink / raw) To: zsh-workers On Fri, 2022-04-15 at 15:27 -0700, Bart Schaefer wrote: > On Thu, Apr 14, 2022 at 2:29 PM Bart Schaefer <schaefer@brasslantern.com> wrote: > > > > [I] would prefer that another set of eyes review > > the code change. > > To expand on that a bit ... > > The BIN_RETURN branch checks that (trap_return == -2) which is a > pretty specific number. exec.c says: > * This is only active if we are inside a trap, else its value > * is irrelevant. It is initialised to -1 for a function trap and > * -2 for a non-function trap and if negative is decremented as > * we go deeper into functions and incremented as we come back up. > * The value is used to decide if an explicit "return" should cause > * a return from the caller of the trap; it does this by setting > * trap_return to a status (i.e. a non-negative value). > > My interpretation is that, since we are in an explicit "exit" rather > than an explicit "return", we don't really care how trap_return is > set; we're going to force the caller to return, period. I just got back and looked, and it's hard to see how this could make anything worse. > However, the test for whether to increment trap_return again on the > way out is dependent on trap_state == TRAP_STATE_PRIMED, so if we > change trap_state we're never going to increment (or decrement, > actually) trap_return again. (It may, however, get restored from a > stacked value.) I can imagine there being some side-effect of this > from using a mix of "return" and "exit" from traps or from functions > that might be called by traps, but have no good idea how to exercise > such a code path. No, indeed; as long as we accumulate all the tests we can think of for any complicated cases of combined exit and return I think we're doing as well as we can. The only note of caution I can think of is to make sure we're testing enough of the normal cases as well as the funnies. As long as that looks reasonable we can just suck it and see. pws ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-19 18:42 ` Peter Stephenson @ 2022-04-19 18:55 ` Peter Stephenson 2022-04-19 21:22 ` Bart Schaefer 0 siblings, 1 reply; 17+ messages in thread From: Peter Stephenson @ 2022-04-19 18:55 UTC (permalink / raw) To: zsh-workers On Tue, 2022-04-19 at 19:42 +0100, Peter Stephenson wrote: > On Fri, 2022-04-15 at 15:27 -0700, Bart Schaefer wrote: > > On Thu, Apr 14, 2022 at 2:29 PM Bart Schaefer <schaefer@brasslantern.com> wrote: > > > > > > [I] would prefer that another set of eyes review > > > the code change. > > > > To expand on that a bit ... > > > > The BIN_RETURN branch checks that (trap_return == -2) which is a > > pretty specific number. exec.c says: > > * This is only active if we are inside a trap, else its value > > * is irrelevant. It is initialised to -1 for a function trap and > > * -2 for a non-function trap and if negative is decremented as > > * we go deeper into functions and incremented as we come back up. > > * The value is used to decide if an explicit "return" should cause > > * a return from the caller of the trap; it does this by setting > > * trap_return to a status (i.e. a non-negative value). > > > > My interpretation is that, since we are in an explicit "exit" rather > > than an explicit "return", we don't really care how trap_return is > > set; we're going to force the caller to return, period. > > I just got back and looked, and it's hard to see how this could make > anything worse. Very minor comment (entirely cosmetic): it would probably be good practice to check trap_state != TRAP_STATE_INACTIVE rather than just trap_state. Does the following look reasonable? In fact, it might be even more logical just to check for PRIMED. With hindsight, "primed" isn't a great choice of word, it doesn't indicate what state we are actually at in trap processing, but without following this all through in more detail I wouldn't like to suggest another. (And the resulting TRAP_STATE_WE_DID_THIS_BUT_WE_HAVENT_YET_DONE_THIS_BECAUSE_WERE_WAITING_FOR_THIS might not be any better...) pws diff --git a/Src/builtin.c b/Src/builtin.c index b93466ba5..88d69e070 100644 --- a/Src/builtin.c +++ b/Src/builtin.c @@ -5720,7 +5720,11 @@ bin_break(char *name, char **argv, UNUSED(Options ops), int func) * a bad job. */ if (stopmsg || (zexit(0, ZEXIT_DEFERRED), !stopmsg)) { - if (trap_state) + /* + * If the trap is primed but we've hit an explicit exit, + * we should skip any further handling and bail out now. + */ + if (trap_state != TRAP_STATE_INACTIVE) trap_state = TRAP_STATE_FORCE_RETURN; retflag = 1; breaks = loops; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-19 18:55 ` Peter Stephenson @ 2022-04-19 21:22 ` Bart Schaefer 2022-04-20 8:16 ` Peter Stephenson 0 siblings, 1 reply; 17+ messages in thread From: Bart Schaefer @ 2022-04-19 21:22 UTC (permalink / raw) To: Peter Stephenson; +Cc: Zsh hackers list On Tue, Apr 19, 2022 at 11:57 AM Peter Stephenson <p.w.stephenson@ntlworld.com> wrote: > > + /* > + * If the trap is primed but we've hit an explicit exit, > + * we should skip any further handling and bail out now. > + */ > + if (trap_state != TRAP_STATE_INACTIVE) > trap_state = TRAP_STATE_FORCE_RETURN; Isn't that exactly equivalent to what I had? TRAP_STATE_INACTIVE is zero, isn't it? Either we want (trap_state == TRAP_STATE_PRIMED) ... or we can go with the above, but it's cosmetic only. I think. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: using trap function to cleanup and exit? 2022-04-19 21:22 ` Bart Schaefer @ 2022-04-20 8:16 ` Peter Stephenson 0 siblings, 0 replies; 17+ messages in thread From: Peter Stephenson @ 2022-04-20 8:16 UTC (permalink / raw) To: zsh workers > On 19 April 2022 at 22:22 Bart Schaefer <schaefer@brasslantern.com> wrote: > On Tue, Apr 19, 2022 at 11:57 AM Peter Stephenson > <p.w.stephenson@ntlworld.com> wrote: > > > > + /* > > + * If the trap is primed but we've hit an explicit exit, > > + * we should skip any further handling and bail out now. > > + */ > > + if (trap_state != TRAP_STATE_INACTIVE) > > trap_state = TRAP_STATE_FORCE_RETURN; > > Isn't that exactly equivalent to what I had? TRAP_STATE_INACTIVE is > zero, isn't it? Correct, it was purely for clarity to keep the logic in terms of states. pws ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-04-20 8:16 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-10 15:46 using trap function to cleanup and exit? Greg Klanderman 2022-04-10 15:57 ` Peter Stephenson 2022-04-10 16:12 ` Greg Klanderman 2022-04-10 16:30 ` Greg Klanderman 2022-04-10 18:15 ` Lawrence Velázquez 2022-04-14 5:13 ` Greg Klanderman 2022-04-10 20:49 ` Bart Schaefer 2022-04-10 20:55 ` Bart Schaefer 2022-04-14 5:57 ` Greg Klanderman 2022-04-14 21:29 ` Bart Schaefer 2022-04-14 23:35 ` Bart Schaefer 2022-04-17 17:08 ` Daniel Shahaf 2022-04-15 22:27 ` Bart Schaefer 2022-04-19 18:42 ` Peter Stephenson 2022-04-19 18:55 ` Peter Stephenson 2022-04-19 21:22 ` Bart Schaefer 2022-04-20 8:16 ` Peter Stephenson
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).