zsh-workers
 help / color / mirror / code / Atom feed
* 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 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 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 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 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-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-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).