From c5c39f4e348dc4759638927343a36e79f2da7bb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Wed, 13 Mar 2019 13:24:09 +0100 Subject: test-lib: fix interrupt handling with 'dash' and '--verbose-log -x' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano diff --git a/t/test-lib.sh b/t/test-lib.sh index 4e79e14..ab3de00 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 -- cgit v0.10.2-6-g49f6 From 80a539acf6663ec97bfbf552faa32105bd7f599f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Wed, 13 Mar 2019 13:24:10 +0100 Subject: t/lib-git-daemon: make sure to kill the 'git-daemon' process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After 'start_git_daemon' starts 'git daemon' (note the space in the middle) in the background, it saves the background process' PID, so the daemon can be stopped at the end of the test script. However, 'git-daemon' is not a builtin but a dashed external command, which means that the dashless 'git daemon' executes the dashed 'git-daemon' command, and, consequently, the PID recorded is not the PID of the "real" daemon process, but that of the main 'git' wrapper. Now, if a test script involving 'git daemon' is interrupted by ctrl-C, then only the main 'git' process is stopped, but the real daemon process tends to survive somehow, and keeps on running in the background indefinitely, keeping the daemon's port to itself, and thus preventing subsequent runs of the same test script. Work this around by running 'git daemon' with the '--pidfile=...' option to save the PID of the real daemon process, and kill that process in 'stop_git_daemon' as well. Signed-off-by: SZEDER Gábor Signed-off-by: Junio C Hamano diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh index 79db3b7..6dab876 100644 --- a/t/lib-git-daemon.sh +++ b/t/lib-git-daemon.sh @@ -31,6 +31,7 @@ fi test_set_port LIB_GIT_DAEMON_PORT GIT_DAEMON_PID= +GIT_DAEMON_PIDFILE="$PWD"/daemon.pid GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo GIT_DAEMON_HOST_PORT=127.0.0.1:$LIB_GIT_DAEMON_PORT GIT_DAEMON_URL=git://$GIT_DAEMON_HOST_PORT @@ -49,7 +50,7 @@ start_git_daemon() { mkfifo git_daemon_output ${LIB_GIT_DAEMON_COMMAND:-git daemon} \ --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \ - --reuseaddr --verbose \ + --reuseaddr --verbose --pid-file="$GIT_DAEMON_PIDFILE" \ --base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \ "$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \ >&3 2>git_daemon_output & @@ -88,8 +89,9 @@ stop_git_daemon() { then error "git daemon exited with status: $ret" fi + kill "$(cat "$GIT_DAEMON_PIDFILE")" 2>/dev/null GIT_DAEMON_PID= - rm -f git_daemon_output + rm -f git_daemon_output "$GIT_DAEMON_PIDFILE" } # A stripped-down version of a netcat client, that connects to a "host:port" -- cgit v0.10.2-6-g49f6 From 900721e15c43742955878dbbdb24c5a2ce262630 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 13 Mar 2019 13:24:11 +0100 Subject: test-lib: introduce 'test_atexit' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When running Apache, 'git daemon', or p4d, we want to kill them at the end of the test script, otherwise a leftover daemon process will keep its port open indefinitely, and thus will interfere with subsequent executions of the same test script. So far, we stop these daemon processes "manually", i.e.: - by registering functions or commands in the trap on EXIT to stop the daemon while preserving the last seen exit code before the trap (to deal with a failure when run with '--immediate' or with interrupts by ctrl-C), - and by invoking these functions/commands last thing before 'test_done' (and sometimes restoring the test framework's default trap on EXIT, to prevent the daemons from being killed twice). On one hand, we do this inconsistently, e.g. 'git p4' tests invoke different functions in the trap on EXIT and in the last test before 'test_done', and they neither restore the test framework's default trap on EXIT nor preserve the last seen exit code. On the other hand, this is error prone, because, as shown in a previous patch in this series, any output from the cleanup commands in the trap on EXIT can prevent a proper cleanup when a test script run with '--verbose-log' and certain shells, notably 'dash', is interrupted. Let's introduce 'test_atexit', which is loosely modeled after 'test_when_finished', but has a broader scope: rather than running the commands after the current test case, run them when the test script finishes, and also run them when the test is interrupted, or exits early in case of a failure while the '--immediate' option is in effect. When running the cleanup commands at the end of a successful test, then they will be run in 'test_done' before it removes the trash directory, i.e. the cleanup commands will still be able to access any pidfiles or socket files in there. When running the cleanup commands after an interrupt or failure with '--immediate', then they will be run in the trap on EXIT. In both cases they will be run in 'test_eval_', i.e. both standard error and output of all cleanup commands will go where they should according to the '-v' or '--verbose-log' options, and thus won't cause any troubles when interrupting a test script run with '--verbose-log'. Signed-off-by: Johannes Schindelin Signed-off-by: SZEDER Gábor Signed-off-by: Junio C Hamano diff --git a/t/README b/t/README index 7a3d582..0b03175 100644 --- a/t/README +++ b/t/README @@ -862,6 +862,26 @@ library for your script to use. ... ' + - test_atexit