@matheusbsbs / matheus-bagliano / commits / 07353d9042

git p4 test: clean up the p4d cleanup functions

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 <szeder.dev@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

SZEDER Gábor committed Mar 13, 2019 at 13:24 UTC 07353d9042f19fec749ec8413ff7e6824460ee59
2 files changed +12 -28
t/lib-git-p4.sh
+10 -26
@@ -67,12 +67,8 @@ cli="$TRASH_DIRECTORY/cli"
67 git="$TRASH_DIRECTORY/git"
68 pidfile="$TRASH_DIRECTORY/p4d.pid"
69
70 -# Sometimes "prove" seems to hang on exit because p4d is still running
71 -cleanup () {
72 - if test -f "$pidfile"
73 - then
74 - kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
75 - fi
70 +stop_p4d_and_watchdog () {
71 + kill -9 $p4d_pid $watchdog_pid
72 }
73
74 # git p4 submit generates a temp file, which will
@@ -87,7 +83,7 @@ start_p4d () {
83 # Don't register and then run the same atexit handlers several times.
84 if test -z "$registered_stop_p4d_atexit_handler"
85 then
90 - test_atexit 'kill_p4d; cleanup'
86 + test_atexit 'stop_p4d_and_watchdog'
87 registered_stop_p4d_atexit_handler=AlreadyDone
88 fi
89
@@ -100,6 +96,7 @@ start_p4d () {
96 echo $! >"$pidfile"
97 }
98 ) &&
99 + p4d_pid=$(cat "$pidfile")
100
101 # This gives p4d a long time to start up, as it can be
102 # quite slow depending on the machine. Set this environment
@@ -107,14 +104,13 @@ start_p4d () {
104 # an automated test setup. If the p4d process dies, that
105 # will be caught with the "kill -0" check below.
106 i=${P4D_START_PATIENCE:-300}
110 - pid=$(cat "$pidfile")
107
108 timeout=$(($(time_in_seconds) + $P4D_TIMEOUT))
109 while true
110 do
111 if test $(time_in_seconds) -gt $timeout
112 then
117 - kill -9 $pid
113 + kill -9 $p4d_pid
114 exit 1
115 fi
116 sleep 1
@@ -131,7 +127,7 @@ start_p4d () {
127 break
128 fi
129 # fail if p4d died
134 - kill -0 $pid 2>/dev/null || break
130 + kill -0 $p4d_pid 2>/dev/null || break
131 echo waiting for p4d to start
132 sleep 1
133 i=$(( $i - 1 ))
@@ -178,22 +174,10 @@ retry_until_success () {
174 done
175 }
176
181 -retry_until_fail () {
182 - timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
183 - until ! "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
184 - do
185 - sleep 1
186 - done
187 -}
188 -
189 -kill_p4d () {
190 - pid=$(cat "$pidfile")
191 - retry_until_fail kill $pid
192 - retry_until_fail kill -9 $pid
193 - # complain if it would not die
194 - test_must_fail kill $pid >/dev/null 2>&1 &&
195 - rm -rf "$db" "$cli" "$pidfile" &&
196 - retry_until_fail kill -9 $watchdog_pid
177 +stop_and_cleanup_p4d () {
178 + kill -9 $p4d_pid $watchdog_pid
179 + wait $p4d_pid
180 + rm -rf "$db" "$cli" "$pidfile"
181 }
182
183 cleanup_git () {
t/t9801-git-p4-branch.sh
+2 -2
@@ -151,7 +151,7 @@ test_expect_success 'import depot, branch detection, branchList branch definitio
151 '
152
153 test_expect_success 'restart p4d' '
154 - kill_p4d &&
154 + stop_and_cleanup_p4d &&
155 start_p4d
156 '
157
@@ -505,7 +505,7 @@ test_expect_success 'use-client-spec detect-branches skips files in branches' '
505 '
506
507 test_expect_success 'restart p4d' '
508 - kill_p4d &&
508 + stop_and_cleanup_p4d &&
509 start_p4d
510 '
511