Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Dec 6, 2018, 23:10 UTC
Message-ID: <20181206231005.GP30222@szeder.dev>
In-Reply-To: <20181205215621.GE19936@sigill.intra.peff.net>

On Wed, Dec 05, 2018 at 04:56:21PM -0500, Jeff King wrote:

Show 7 quoted lines

> Could we just kill them all?
> 
> I guess it's a little tricky, because $! is going to give us the pid of
> each subshell. We actually want to kill the test sub-process. That takes
> a few contortions, but the output is nice (every sub-job immediately
> says "ABORTED ...", and the final process does not exit until the whole
> tree is done):

It's trickier than that:

  - Nowadays our test lib translates SIGINT to exit, but not TERM (or
    HUP, in case a user decides to close the terminal window), which
    means that if the test runs any daemons in the background, then
    those won't be killed when the stress test is aborted.

    This is easy enough to address, and I've been meaning to do this
    in an other patch series anyway.

  - With the (implied) '--verbose-log' the process killed in the
    background subshell's SIGTERM handler is not the actual test
    process, because '--verbose-log' runs the test again in a piped
    subshell to save its output:
    
      (GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
       echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"

    That 'kill' kills the "outer" shell process.
    And though I get "ABORTED..." immediately and I get back my
    prompt right away, the commands involved in the above construct
    are still running in the background:

      $ ./t3903-stash.sh --stress=1
      ^CABORTED  0.0
      $ ps a |grep t3903
      1280 pts/17   S      0:00 /bin/sh ./t3903-stash.sh --stress=1
      1281 pts/17   S      0:00 tee -a <...>/test-results/t3903-stash.stress-0.out
      1282 pts/17   S      0:00 /bin/sh ./t3903-stash.sh --stress=1
      4173 pts/17   S+     0:00 grep t3903

    I see this behavior with all shells I tried.
    I haven't yet started to think it through why this happens :)

    Not implying '--verbose-log' but redirecting the test script's
    output, like you did in your 'stress' script, seems to work in
    dash, ksh, and ks93, but not in Bash v4.3 or later, where, for
    whatever reason, the subshell get SIGINT before the SIGTERM would
    arrive.
    While we could easily handle SIGINT in the subshell with the same
    signal handler as SIGTERM, it bothers me that something
    fundamental is wrong here.
    Furthermore, then we should perhaps forbid '--stress' in
    combination with '--verbose-log' or '--tee'.

Show 50 quoted lines

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 9b7f687396..357dead3ff 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -98,8 +98,9 @@ done,*)
>  	mkdir -p "$(dirname "$TEST_RESULTS_BASE")"
>  	stressfail="$TEST_RESULTS_BASE.stress-failed"
>  	rm -f "$stressfail"
> -	trap 'echo aborted >"$stressfail"' TERM INT HUP
> +	trap 'echo aborted >"$stressfail"; kill $job_pids 2>/dev/null; wait' TERM INT HUP
>  
> +	job_pids=
>  	job_nr=0
>  	while test $job_nr -lt "$job_count"
>  	do
> @@ -108,10 +109,13 @@ done,*)
>  			GIT_TEST_STRESS_JOB_NR=$job_nr
>  			export GIT_TEST_STRESS_STARTED GIT_TEST_STRESS_JOB_NR
>  
> +			trap 'kill $test_pid 2>/dev/null' TERM
> +
>  			cnt=0
>  			while ! test -e "$stressfail"
>  			do
> -				if $TEST_SHELL_PATH "$0" "$@" >/dev/null 2>&1
> +				$TEST_SHELL_PATH "$0" "$@" >/dev/null 2>&1 & test_pid=$!
> +				if wait
>  				then
>  					printf >&2 "OK   %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
>  				elif test -f "$stressfail" &&
> @@ -124,16 +128,11 @@ done,*)
>  				fi
>  				cnt=$(($cnt + 1))
>  			done
> -		) &
> +		) & job_pids="$job_pids $!"
>  		job_nr=$(($job_nr + 1))
>  	done
>  
> -	job_nr=0
> -	while test $job_nr -lt "$job_count"
> -	do
> -		wait
> -		job_nr=$(($job_nr + 1))
> -	done
> +	wait
>  
>  	if test -f "$stressfail" && test "$(cat "$stressfail")" != "aborted"
>  	then
>