git p4 test: clean up the p4d cleanup functions

szeder · gitster · commit 07353d9042f1 · 2019-03-14T12:34:39.000+09:00
Confusingly, the 'git p4' tests used two cleanup functions:

  - 'kill_p4d' was run in the last test before 'test_done', and it not
    only killed 'p4d', but it killed the watchdog process, and cleaned
    up after 'p4d' as well by removing all directories used by the P4
    daemon and client.

    This cleanup is not necessary right before 'test_done', because
    the whole trash directory is about to get removed anyway, but it
    is necessary in 't9801-git-p4-branch.sh', which uses 'kill_p4d' to
    stop 'p4d' before re-starting it in the middle of the test script.

  - 'cleanup' was run in the trap on EXIT, and it killed 'p4d', but,
    it didn't kill the watchdog process, and, contrarily to its name,
    didn't perform any cleanup whatsoever.

Make it clearer what's going on by renaming and simplifying the
cleanup functions, so in the end we'll have:

  - 'stop_p4d_and_watchdog' replaces 'cleanup' as it will try to live
    up to its name and stop both the 'p4d' and the watchdog processes,
    and as the sole function registered with 'test_atexit' it will be
    responsible for no leaving any stray processes behind after 'git p4'
    tests were finished or interrupted.

  - 'stop_and_cleanup_p4d' replaces 'kill_p4d' as it will stop 'p4d'
    (and the watchdog) and remove all directories used by the P4
    daemon and cliean, so it can be used mid-script to stop and then
    re-start 'p4d'.

Note that while 'cleanup' sent a single SIGKILL to 'p4d', 'kill_p4d'
was quite brutal, as it first sent SIGTERM to the daemon repeatedly,
either until its pid disappeared or until a given timeout was up, and
then it sent SIGKILL repeatedly, for good measure.  This is overkill
(pardon the pun): a single SIGKILL should be able to take down any
process in a sensible state, and if a process were to somehow end up
stuck in the dreaded uninterruptible sleep state then even repeated
SIGKILLs won't bring immediate help.  So ditch all the repeated
SIGTERM/SIGKILL parts, and use a single SIGKILL to stop 'p4d', and
make sure that there are no races between asynchron signal delivery
and subsequent restart of 'p4d' by waiting for it to die.

With this change the 'retry_until_fail' helper has no callers left,
remove it.

Signed-off-by: SZEDER Gábor &lt;szeder.dev@gmail.com&gt;
Signed-off-by: Junio C Hamano &lt;gitster@pobox.com&gt;
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
@@ -67,12 +67,8 @@ cli="$TRASH_DIRECTORY/cli"
 git="$TRASH_DIRECTORY/git"
 pidfile="$TRASH_DIRECTORY/p4d.pid"
 
-# Sometimes "prove" seems to hang on exit because p4d is still running
-cleanup () {
-	if test -f "$pidfile"
-	then
-		kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
-	fi
+stop_p4d_and_watchdog () {
+	kill -9 $p4d_pid $watchdog_pid
 }
 
 # git p4 submit generates a temp file, which will
@@ -87,7 +83,7 @@ start_p4d () {
 	# Don't register and then run the same atexit handlers several times.
 	if test -z "$registered_stop_p4d_atexit_handler"
 	then
-		test_atexit 'kill_p4d; cleanup'
+		test_atexit 'stop_p4d_and_watchdog'
 		registered_stop_p4d_atexit_handler=AlreadyDone
 	fi
 
@@ -100,21 +96,21 @@ start_p4d () {
 			echo $! >"$pidfile"
 		}
 	) &&
+	p4d_pid=$(cat "$pidfile")
 
 	# This gives p4d a long time to start up, as it can be
 	# quite slow depending on the machine.  Set this environment
 	# variable to something smaller to fail faster in, say,
 	# an automated test setup.  If the p4d process dies, that
 	# will be caught with the "kill -0" check below.
 	i=${P4D_START_PATIENCE:-300}
-	pid=$(cat "$pidfile")
 
 	timeout=$(($(time_in_seconds) + $P4D_TIMEOUT))
 	while true
 	do
 		if test $(time_in_seconds) -gt $timeout
 		then
-			kill -9 $pid
+			kill -9 $p4d_pid
 			exit 1
 		fi
 		sleep 1
@@ -131,7 +127,7 @@ start_p4d () {
 			break
 		fi
 		# fail if p4d died
-		kill -0 $pid 2>/dev/null || break
+		kill -0 $p4d_pid 2>/dev/null || break
 		echo waiting for p4d to start
 		sleep 1
 		i=$(( $i - 1 ))
@@ -178,22 +174,10 @@ retry_until_success () {
 	done
 }
 
-retry_until_fail () {
-	timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
-	until ! "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
-	do
-		sleep 1
-	done
-}
-
-kill_p4d () {
-	pid=$(cat "$pidfile")
-	retry_until_fail kill $pid
-	retry_until_fail kill -9 $pid
-	# complain if it would not die
-	test_must_fail kill $pid >/dev/null 2>&1 &&
-	rm -rf "$db" "$cli" "$pidfile" &&
-	retry_until_fail kill -9 $watchdog_pid
+stop_and_cleanup_p4d () {
+	kill -9 $p4d_pid $watchdog_pid
+	wait $p4d_pid
+	rm -rf "$db" "$cli" "$pidfile"
 }
 
 cleanup_git () {
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
@@ -151,7 +151,7 @@ test_expect_success 'import depot, branch detection, branchList branch definitio
 '
 
 test_expect_success 'restart p4d' '
-	kill_p4d &&
+	stop_and_cleanup_p4d &&
 	start_p4d
 '
 
@@ -505,7 +505,7 @@ test_expect_success 'use-client-spec detect-branches skips files in branches' '
 '
 
 test_expect_success 'restart p4d' '
-	kill_p4d &&
+	stop_and_cleanup_p4d &&
 	start_p4d
 '
 

Original file line number	Diff line number	Diff line change
`@@ -151,7 +151,7 @@ test_expect_success 'import depot, branch detection, branchList branch definitio`
`151`	`151`	`'`
`152`	`152`
`153`	`153`	`test_expect_success 'restart p4d' '`
`154`		`- kill_p4d &&`
	`154`	`+ stop_and_cleanup_p4d &&`
`155`	`155`	`start_p4d`
`156`	`156`	`'`
`157`	`157`
`@@ -505,7 +505,7 @@ test_expect_success 'use-client-spec detect-branches skips files in branches' '`
`505`	`505`	`'`
`506`	`506`
`507`	`507`	`test_expect_success 'restart p4d' '`
`508`		`- kill_p4d &&`
	`508`	`+ stop_and_cleanup_p4d &&`
`509`	`509`	`start_p4d`
`510`	`510`	`'`
`511`	`511`