threads / discuss / 38560

t5570 trap use in start/stop_git_daemon

Subject: t5570 trap use in start/stop_git_daemon

## tl;dr

5 messages between Feb 12, 2015 and Feb 13, 2015.

replies: 4people: 3as markdown or json

Randall S. Becker· Feb 12, 2015, 20:31 UTC · lore

On the NonStop port, we found that trap was causing an issue with test success for t5570. When start_git_daemon completes, the shell (ksh,bash) on this platform is sending a signal 0 that is being caught and acted on by the trap command within the start_git_daemon and stop_git_daemon functions. I am taking this up with the operating system group, but in any case, it may be appropriate to include a trap reset at the end of both functions, as below. I verified this change on SUSE Linux.

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index bc4b341..543e98a 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -62,6 +62,7 @@ start_git_daemon() {
                test_skip_or_die $GIT_TEST_GIT_DAEMON \
                        "git daemon failed to start"
       fi
+       trap '' EXIT
}

stop_git_daemon() {
@@ -84,4 +85,6 @@ stop_git_daemon() {
        fi
        GIT_DAEMON_PID=
        rm -f git_daemon_output
+
+       trap '' EXIT
}

Cheers,
Randall
-- Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)/NonStop(211288444200000000)
-- In real life, I talk too much.

Jeff King· Feb 13, 2015, 07:44 UTC · re: Randall S. Becker · lore

Re: t5570 trap use in start/stop_git_daemon

On Thu, Feb 12, 2015 at 03:31:12PM -0500, Randall S. Becker wrote:

Show 5 quoted lines

> On the NonStop port, we found that trap was causing an issue with test
> success for t5570. When start_git_daemon completes, the shell (ksh,bash) on
> this platform is sending a signal 0 that is being caught and acted on by the
> trap command within the start_git_daemon and stop_git_daemon functions. I am
> taking this up with the operating system group,

Yeah, that seems wrong. If it were a subshell, even, I could see some argument for it, but it seems odd to trap 0 when a function returns (bash does have a RETURN trap, which AFAIK is bash-specific, but it should not trigger a 0-trap).

Show 14 quoted lines

> but in any case, it may be
> appropriate to include a trap reset at the end of both functions, as below.
> I verified this change on SUSE Linux.
> 
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index bc4b341..543e98a 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -62,6 +62,7 @@ start_git_daemon() {
>                 test_skip_or_die $GIT_TEST_GIT_DAEMON \
>                         "git daemon failed to start"
>        fi
> +       trap '' EXIT
> }

I don't think this is the right thing to do. That trap is meant to live beyond the function's return. Without it, there is nothing to clean up the running git-daemon if we exit the test script prematurely (e.g., by a test failing in immediate-mode). We pollute the environment with a running process which would cause subsequent test runs to fail.

Show 8 quoted lines

> stop_git_daemon() {
> @@ -84,4 +85,6 @@ stop_git_daemon() {
>         fi
>         GIT_DAEMON_PID=
>         rm -f git_daemon_output
> +
> +       trap '' EXIT
> }

This one is slightly less bad, in that we are dropping our daemon-specific cleanup here anyway. But the appropriate trap is still:

  trap 'die' EXIT

which we set earlier in the function. Without it, the test harness's ability to detect a premature failure is lost.

So I do not know quite what is going on with your shell, but turning off the traps in these functions is definitely not an acceptable (general) workaround; it makes things much worse on working platforms.

-Peff

Jeff King· Feb 13, 2015, 08:03 UTC · re: Jeff King · lore

Re: t5570 trap use in start/stop_git_daemon

On Fri, Feb 13, 2015 at 02:44:03AM -0500, Jeff King wrote:

Show 12 quoted lines

> On Thu, Feb 12, 2015 at 03:31:12PM -0500, Randall S. Becker wrote:
> 
> > On the NonStop port, we found that trap was causing an issue with test
> > success for t5570. When start_git_daemon completes, the shell (ksh,bash) on
> > this platform is sending a signal 0 that is being caught and acted on by the
> > trap command within the start_git_daemon and stop_git_daemon functions. I am
> > taking this up with the operating system group,
> 
> Yeah, that seems wrong. If it were a subshell, even, I could see some
> argument for it, but it seems odd to trap 0 when a function returns
> (bash does have a RETURN trap, which AFAIK is bash-specific, but it
> should not trigger a 0-trap).

Hmm, today I learned something new about ksh. Apparently when you use the "function" keyword to define a function like:

  function foo {
    trap 'echo trapped' EXIT
  }
  echo before
  foo
  echo after

then the trap runs when the function exits! If you declare the same function as:

  foo() {
    trap 'echo trapped' EXIT
  }

it behaves differently. POSIX shell does not have the function keyword, of course, and we are not using it here. Bash _does_ have the function keyword, but seems to behave POSIX-y even when it is present. I.e., running the first script:

  $ ksh foo.sh
  before
  trapped
  after

  $ bash foo.sh
  before
  after
  trapped

  $ dash foo.sh
  foo.sh: 3: foo.sh: function: not found
  foo.sh: 5: foo.sh: Syntax error: "}" unexpected

Switching to the second form, all three produce:

  before
  after
  trapped

I don't know if that is all helpful to your bug-tracking or analysis, but for whatever reason it looks like your ksh is using localized traps for both forms of function. But as far as I know, bash has never behaved that way (I just grepped its CHANGES file for mentions of trap and found nothing likely).

-Peff

Joachim Schmitz· Feb 13, 2015, 08:57 UTC · re: Jeff King · lore

Re: t5570 trap use in start/stop_git_daemon

Jeff King <peff <at> peff.net> writes:

Show 5 quoted lines

> 
> On Fri, Feb 13, 2015 at 02:44:03AM -0500, Jeff King wrote:
> 
> > On Thu, Feb 12, 2015 at 03:31:12PM -0500, Randall S. Becker wrote:
> >

<snip>

Show 50 quoted lines

> Hmm, today I learned something new about ksh. Apparently when you use
> the "function" keyword to define a function like:
> 
>   function foo {
>     trap 'echo trapped' EXIT
>   }
>   echo before
>   foo
>   echo after
> 
> then the trap runs when the function exits! If you declare the same
> function as:
> 
>   foo() {
>     trap 'echo trapped' EXIT
>   }
> 
> it behaves differently. POSIX shell does not have the function keyword,
> of course, and we are not using it here. Bash _does_ have the function
> keyword, but seems to behave POSIX-y even when it is present. I.e.,
> running the first script:
> 
>   $ ksh foo.sh
>   before
>   trapped
>   after
> 
>   $ bash foo.sh
>   before
>   after
>   trapped
> 
>   $ dash foo.sh
>   foo.sh: 3: foo.sh: function: not found
>   foo.sh: 5: foo.sh: Syntax error: "}" unexpected
> 
> Switching to the second form, all three produce:
> 
>   before
>   after
>   trapped
> 
> I don't know if that is all helpful to your bug-tracking or analysis,
> but for whatever reason it looks like your ksh is using localized traps
> for both forms of function. But as far as I know, bash has never behaved
> that way (I just grepped its CHANGES file for mentions of trap and found
> nothing likely).
> 
> -Peff
>

Both versions produce your first output on our platform

$ ksh foo1.sh before trapped after $ bash foo1.sh before after trapped $ ksh foo2.sh before trapped after $ bash foo2.sh before after trapped $

This might have been one (or even _the_) reason why we picked bash as our SHELL_PATH in config.mak.uname (I don't remember, it's more than 2 years ago), not sure which shell Randall's test used?

bye, Jojo

Randall S. Becker· Feb 13, 2015, 12:27 UTC · re: Joachim Schmitz · lore

RE: t5570 trap use in start/stop_git_daemon

On 2015/02/13 3:58AM Joachim Schmitz wrote:

Show 37 quoted lines

>Jeff King <peff <at> peff.net> writes:
> > On Fri, Feb 13, 2015 at 02:44:03AM -0500, Jeff King wrote:
> > On Thu, Feb 12, 2015 at 03:31:12PM -0500, Randall S. Becker wrote:
> > 
><snip> 
>> Hmm, today I learned something new about ksh. Apparently when you use
>> the "function" keyword to define a function like:
>> 
>>   function foo {
>>     trap 'echo trapped' EXIT
>>   }
>>   echo before
>>   foo
>>   echo after
>> 
>> then the trap runs when the function exits! If you declare the same
>> function as:
>> 
>>   foo() {
>>     trap 'echo trapped' EXIT
>>   }
>> 
>> it behaves differently. POSIX shell does not have the function keyword,
>> of course, and we are not using it here. Bash _does_ have the function
>> keyword, but seems to behave POSIX-y even when it is present. I.e.,
>> running the first script:
>> 
>>   $ ksh foo.sh
>>   before
>>   trapped
>>   after
>> 
>>   $ bash foo.sh
> >  before
>>   after
> >  trapped
>>

<snip>

Show 21 quoted lines

>Both versions produce your first output on our platform
>$ ksh foo1.sh
>before
>trapped
>after
>$ bash foo1.sh
>before
>after
>trapped
>$ ksh foo2.sh
>before
>trapped
>after
>$ bash foo2.sh
>before
>after
>trapped
>$
>This might have been one (or even _the_) reason why we picked bash as our 
>SHELL_PATH in config.mak.uname (I don't remember, it's more than 2 years 
>ago), not sure which shell Randall's test used?

I tested both for trying to get t5570 to work. No matter which, without resetting the trap, function return would kill the git-daemon and the test would fail.