Message ID | 20190313122419.2210-2-szeder.dev@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | tests: introduce 'test_atexit' | expand |
SZEDER Gábor <szeder.dev@gmail.com> writes: > Putting these together, when a test script run with 'dash' and > '--verbose-log -x' is interrupted, then 'dash' tries to write the > trace output from the EXIT trap to the script's original standard > error, but it very likely can't, because the 'tee' downstream of the > pipe is interrupted as well. This causes the shell running the test > script to die because of SIGPIPE, without running any of the commands > in the EXIT trap. So... the clean-up actions do not get a chance to run because the shell that is going to execute would be killed by SIGPIPE because tee that is reading from it goes away? While I like the patch very much, I also wonder if it is possible to tell tee to not to die upon seeing INT, TERM, etc. When the shell upstream of tee exits (after performing clean-up actions), tee that is reading from it will exit, too, and will not be left behind (I do not mean to say it that is a better alternative solution---I am just trying to see if I read the problem correctly from the description given above). > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 8665b0a9b6..db3875d1e4 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -631,7 +631,10 @@ die () { > > GIT_EXIT_OK= > trap 'die' EXIT > -trap 'exit $?' INT TERM HUP > +# Disable '-x' tracing, because with some shells, notably dash, it > +# prevents running the cleanup commands when a test script run with > +# '--verbose-log -x' is interrupted. > +trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP > > # The user-facing functions are loaded from a separate file so that > # test_perf subshells can have them too Thanks.
On Thu, Mar 14, 2019 at 11:18:41AM +0900, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > Putting these together, when a test script run with 'dash' and > > '--verbose-log -x' is interrupted, then 'dash' tries to write the > > trace output from the EXIT trap to the script's original standard > > error, but it very likely can't, because the 'tee' downstream of the > > pipe is interrupted as well. This causes the shell running the test > > script to die because of SIGPIPE, without running any of the commands > > in the EXIT trap. > > So... the clean-up actions do not get a chance to run because the > shell that is going to execute would be killed by SIGPIPE because > tee that is reading from it goes away? Yes. > While I like the patch very much, I also wonder if it is possible to > tell tee to not to die upon seeing INT, TERM, etc. 'tee -i' ignores only INT, but not TERM and HUP. What could work is something like: <re-executing the test script> | ( # Ignore common interrupt signals to prevent 'tee' from dying # while the upstream test process still produces output. trap '' INT TERM HUP tee logfile ) but I'm not sure what should happen with a process ignoring HUP when it still produces output to the terminal after that terminal has been closed, i.e. this 'tee' process. FWIW, In my quick test such a process continued happily without receiving SIGPIPE or other unpleasantness. > When the shell > upstream of tee exits (after performing clean-up actions), tee that > is reading from it will exit, too, and will not be left behind (I do > not mean to say it that is a better alternative solution---I am just > trying to see if I read the problem correctly from the description > given above). > > > diff --git a/t/test-lib.sh b/t/test-lib.sh > > index 8665b0a9b6..db3875d1e4 100644 > > --- a/t/test-lib.sh > > +++ b/t/test-lib.sh > > @@ -631,7 +631,10 @@ die () { > > > > GIT_EXIT_OK= > > trap 'die' EXIT > > -trap 'exit $?' INT TERM HUP > > +# Disable '-x' tracing, because with some shells, notably dash, it > > +# prevents running the cleanup commands when a test script run with > > +# '--verbose-log -x' is interrupted. > > +trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP > > > > # The user-facing functions are loaded from a separate file so that > > # test_perf subshells can have them too > > Thanks.
diff --git a/t/test-lib.sh b/t/test-lib.sh index 8665b0a9b6..db3875d1e4 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -631,7 +631,10 @@ die () { GIT_EXIT_OK= trap 'die' EXIT -trap 'exit $?' INT TERM HUP +# Disable '-x' tracing, because with some shells, notably dash, it +# prevents running the cleanup commands when a test script run with +# '--verbose-log -x' is interrupted. +trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP # The user-facing functions are loaded from a separate file so that # test_perf subshells can have them too
When a test script run with 'dash' and '--verbose-log -x' is interrupted by ctrl-C, SIGTERM, or closing the terminal window, then most of the time the registered EXIT trap actions are not executed. This is an annoying issue with tests involving daemons, because they should run cleanup commands to kill those daemon processes in the trap on EXIT, but since these cleanup commands are not executed, the daemons are left alive and keep their port open, thus interfering with subsequent execution of the same test script. The cause of this issue is the subtle combination of several factors (bear with me, or skip over the indented part): - Even when the test script is interrupted, the cleanup commands are not run in the trap on INT, TERM, or HUP, but in the trap on EXIT after the trap on the signals invokes 'exit' [1]. - According to POSIX [2]: "The environment in which the shell executes a trap on EXIT shall be identical to the environment immediately after the last command executed before the trap on EXIT was taken." Pertinent to the issue at hand is that all open file descriptors and the state of '-x' tracing should be preserved. All shells I've tried [3] preserve '-x'. Unfortunately, however: - 'dash' doesn't conform to this when it comes to open file descriptors: even when standard output and/or error are redirected somewhere when 'exit' is invoked, anything written to them in the trap on EXIT goes to the script's original stdout and stderr [4]. We can't dismiss this with a simple "it doesn't conform to POSIX, so we don't care", because 'dash' is the default /bin/sh in some of the more popular Linux distros. - As far as I can tell, POSIX doesn't explicitly say anything about the environment of trap actions for various signals. In practice it seems that most shells behave sensibly and preserve both open file descriptors and the state of '-x' tracing for the traps on INT, TERM, and HUP, including even 'dash'. The exceptions are 'mksh' and 'lksh': they do preserve '-x', but not the open file descriptors. - When a test script run with '-x' tracing enabled is interrupted, then it's very likely that the signal arrives mid-test, i.e.: - while '-x' tracing is enabled, and, consequently, our trap actions on INT, TERM, HUP, and EXIT will produce trace output as well. - while standard output and error are redirected to a log file, to the test script's original standard output and error, or to /dev/null, depending on whether the test script was run with '--verbose-log', '-v', or neither. According to the above, we can't rely on these redirections still be in effect when running the traps on INT, TERM, HUP, and/or EXIT. - When a test script is run with '--verbose-log', then the test script is re-executed with its standard output and error piped into 'tee', in order to send the "regular" non-verbose test's output both to the terminal and to the log file. When the test is interrupted, then the signal interrupts the downstream 'tee' as well. Putting these together, when a test script run with 'dash' and '--verbose-log -x' is interrupted, then 'dash' tries to write the trace output from the EXIT trap to the script's original standard error, but it very likely can't, because the 'tee' downstream of the pipe is interrupted as well. This causes the shell running the test script to die because of SIGPIPE, without running any of the commands in the EXIT trap. Disable '-x' tracing in the trap on INT, TERM, and HUP to avoid this issue, as it disables tracing in the chained trap on EXIT as well. Wrap it in a '{ ... } 2>/dev/null' block, so the trace of the command disabling the tracing doesn't go to standard error either [5]. Note that it's not only '-x' tracing that can be problematic, but any shell builtin, e.g. 'echo', that writes to standard output or error in the trap on EXIT, while a test running with 'dash' and '--verbose-log' (even without '-x') is interrupted. As far as I can tell, this is not an issue at the moment: - The cleanup commands to stop the credential-helper, Apache, or 'p4d' don't use any such shell builtins. - stop_git_daemon() does use 'say' and 'error', both wrappers around 'echo', but it redirects 'say' to fd 3, i.e. to the log file, and while 'error' does write to standard output, it comes only after the daemon was killed. - The non-builtin commands that actually stop the daemons ('kill', 'apache2 -k stop', 'git credential-cache exit') are silent, so they won't get SIGPIPE before finishing their job. [1] The trap on EXIT must run cleanup commands, because we want to stop any daemons when a test script run with '--immediate' fails and exits early with error. By chaining up the trap on signals to the trap on EXIT we can deal with cleanup commands a bit simpler, because the tests involving daemons only have to set a single trap. [2] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#trap [3] The shells I tried: dash, Bash, ksh, ksh93, mksh, lksh, yash, BusyBox sh, FreeBSD /bin/sh, NetBSD /bin/sh. [4] $ cat trap-output.sh #!/bin/sh trap "echo output; echo error >&2" EXIT { exit; } >OUT 2>ERR $ dash ./trap-output.sh output error $ wc -c OUT ERR 0 OUT 0 ERR On a related note, 'ksh', 'ksh93', and BusyBox sh don't conform to the specs in this respect, either. [5] This '{ set +x; } 2>/dev/null' trick won't help those shells that show trace output for any redirections and don't preserve open file descriptors for the trap on INT, TERM and HUP. The only such shells I'm aware of are 'mksh' and 'lksh'. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- t/test-lib.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)