From: Christian Weisgerber <naddy@mips.inka.de>
Subject: regress: do not use job control, timeout(1)
To: gameoftrees@openbsd.org
Date: Wed, 1 May 2024 22:48:38 +0200

Some regression tests now start a http server in the background:

| test_clone_basic_http() {
| ...
|         timeout 5 ./http-server -p $GOT_TEST_HTTP_PORT $testroot \
|             > $testroot/http-server.log &
|         trap "kill %1" HUP INT QUIT PIPE TERM
| ...
|         kill %1
|         wait %1 # wait for http-server

This has multiple problems:

* Job control in a non-interactive shell is undefined, unless
  explicitly enabled with set +m.  I'm surprised this even works
  as intended.

* The shell will now execute only the trap when receiving one of
  those signals.  In particular, it will no longer terminate itself.
  Also the trap isn't reset, so it continues to be active after the
  background process has already been killed.

* timeout(1) is non-standard.

Since a non-interactive shell doesn't use job control, background
processes share the shell's process group.  If you hit ^C or ^\ in
the terminal, SIGINT or SIGQUIT will be sent to all members of the
process group.  If you close an xterm, SIGHUP will be sent to all
members of the process group.  So the background process will be
killed along with the shell.

Unfortunately some shells ignore SIGINT (bash) and or both SIGINT
and SIGQUIT (FreeBSD sh) for background processes.  We can use a
trap to convert those to SIGTERM.  I think this can be unconditionally
set in common.sh.

I'm using "wait" instead of "wait $!", because some shells report
a "Terminated" in case of the latter.

I think this is worth a try.  It works in my testing on OpenBSD and
FreeBSD.  If people suffer http-server processes hanging around,
we can revisit it.  I admit I'm less confident about this than I'm
usually about shell scripting.

Other ideas?
OK?

-----------------------------------------------
commit de7e55e66a99ba198a15310e7b2707759940b5ca (bg)
from: Christian Weisgerber <naddy@mips.inka.de>
date: Wed May  1 20:26:49 2024 UTC
 
 regress: do not use job control in a non-interactive shell
 
 Also do not use the non-standard timeout(1) utility.
 
diff 7268d4618603a3243f53ced23fc7ccc40f7d0693 de7e55e66a99ba198a15310e7b2707759940b5ca
commit - 7268d4618603a3243f53ced23fc7ccc40f7d0693
commit + de7e55e66a99ba198a15310e7b2707759940b5ca
blob - 95b0e4aeb0e81d800561f45b18ef2f2c3e1b60b9
blob + 6273871137a0c00424113501ff863d8f778cb996
--- regress/cmdline/clone.sh
+++ regress/cmdline/clone.sh
@@ -867,9 +867,8 @@ test_clone_basic_http() {
 	local testurl=http://127.0.0.1:${GOT_TEST_HTTP_PORT}
 	local commit_id=`git_show_head $testroot/repo`
 
-	timeout 5 ./http-server -p $GOT_TEST_HTTP_PORT $testroot \
+	./http-server -p $GOT_TEST_HTTP_PORT $testroot \
 	    > $testroot/http-server.log &
-	trap "kill %1" HUP INT QUIT PIPE TERM
 
 	sleep 1 # server starts up
 
@@ -891,8 +890,8 @@ test_clone_basic_http() {
 		return 1
 	fi
 
-	kill %1
-	wait %1 # wait for http-server
+	kill $!
+	wait # wait for http-server
 
 	got log -l0 -p -r $testroot/repo > $testroot/log-repo
 	ret=$?
blob - fa88b8e9da3292f78931b60f217cfd5ede722f86
blob + 3c1c139985768b06a670f924e3ae138de452cda5
--- regress/cmdline/common.sh
+++ regress/cmdline/common.sh
@@ -33,6 +33,9 @@ export LC_ALL=C
 
 export MALLOC_OPTIONS=S
 
+# Clean up background processes. Some shells catch SIGINT/SIGQUIT.
+trap "trap - TERM; kill -TERM -$$" INT QUIT TERM
+
 git_init()
 {
 	git init -q "$1"
blob - 928c5f4563e3de646b74ef37ef6fd2588857395d
blob + 0c9e24dd93b4a669ec5d2b943cfc25740344952a
--- regress/cmdline/fetch.sh
+++ regress/cmdline/fetch.sh
@@ -2025,9 +2025,8 @@ test_fetch_basic_http() {
 	local testurl=http://127.0.0.1:$GOT_TEST_HTTP_PORT
 	local commit_id=`git_show_head $testroot/repo`
 
-	timeout 5 ./http-server -p $GOT_TEST_HTTP_PORT $testroot \
+	./http-server -p $GOT_TEST_HTTP_PORT $testroot \
 	    > $testroot/http-server.log &
-	trap "kill %1" HUP INT QUIT PIPE TERM
 
 	sleep 1 # server starts up
 
@@ -2060,8 +2059,8 @@ test_fetch_basic_http() {
 		return 1
 	fi
 
-	kill %1
-	wait %1 # wait for http-server
+	kill $!
+	wait # wait for http-server
 
 	echo -n > $testroot/stdout.expected
 
blob - 6e41801df46b386834c2cd1d3bc4d5c039d5f809
blob + e599c6237ac7935ab8bd26579825606c80198bc6
--- regress/cmdline/send.sh
+++ regress/cmdline/send.sh
@@ -1670,9 +1670,8 @@ test_send_basic_http() {
 	local testurl=http://127.0.0.1:$GOT_TEST_HTTP_PORT
 	local commit_id=`git_show_head $testroot/repo`
 
-	timeout 5 ./http-server -p $GOT_TEST_HTTP_PORT $testroot \
+	./http-server -p $GOT_TEST_HTTP_PORT $testroot \
 	    > $testroot/http-server.log &
-	trap "kill %1" HUP INT QUIT PIPE TERM
 
 	sleep 1 # server starts up
 
@@ -1712,8 +1711,8 @@ EOF
 		return 1
 	fi
 
-	kill %1
-	wait %1 # wait for http-server
+	kill $!
+	wait # wait for http-server
 
 	echo "got: http: feature not implemented" > $testroot/stderr.expected
 	cmp -s $testroot/stderr $testroot/stderr.expected

-- 
Christian "naddy" Weisgerber                          naddy@mips.inka.de