Subject: Re: bug#51793: FAIL: tests/misc/env-signal-handler
From: Pádraig Brady <[email-masked]>
To: Andreas Schwab <[email-masked]>
Cc: [email-masked]
Date: Sat, 13 Nov 2021 12:18:39 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

On 13/11/2021 09:46, Andreas Schwab wrote:
> On Nov 12 2021, Pádraig Brady wrote:
> 
>> I've never seen this, and I can't see the race.
> 
> There is an obvious race: if env needs more than .1 seconds to set the
> SIGINT handler.

Indeed.

That fits the pattern where we need a certain delay to pass.
So we can use our retry_delay_ helper here.
I'll apply the attached later.

thanks,
Pádraig
From dd460b0a7ac029a1809ca2ef363701b431e613be Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Sat, 13 Nov 2021 12:15:17 +0000
Subject: [PATCH] tests: avoid rare false failure in env-signal-handler.sh

* tests/misc/env-signal-handler.sh: Use retry_delay_ to
avoid a false failure under load, where env hasn't setup
the SIGINT handling before timeout(1) sends the SIGINT.
Fixes https://bugs.gnu.org/51793
---
 tests/misc/env-signal-handler.sh | 64 +++++++++++++-------------------
 1 file changed, 26 insertions(+), 38 deletions(-)

diff --git a/tests/misc/env-signal-handler.sh b/tests/misc/env-signal-handler.sh
index aa6ec8dac..c4179fe9e 100755
--- a/tests/misc/env-signal-handler.sh
+++ b/tests/misc/env-signal-handler.sh
@@ -82,62 +82,50 @@ compare /dev/null err4 || fail=1
 # env test - block signal handler
 env --block-signal true || fail=1
 
+env_ignore_delay_()
+{
+  local delay="$1"
+
+  # The first 'env' is just to ensure timeout is not a shell built-in.
+  env timeout --verbose --kill-after=.1 --signal=INT $delay \
+    env $env_opt \
+    sleep 10 > /dev/null 2>outt
+  # check only the first two lines from stderr, which are printed by timeout.
+  # (operating systems might add more messages, like "killed").
+  sed -n '1,2p' outt > out || framework_failure_
+  compare exp out
+}
+
 # Baseline test - ignore signal handler
 # -------------------------------------
-# Kill 'sleep' after 1 second with SIGINT - it should terminate (as SIGINT's
-# default action is to terminate a program).
-# (The first 'env' is just to ensure timeout is not the shell's built-in.)
-env timeout --verbose --kill-after=.1 --signal=INT .1 \
-    sleep 10 > /dev/null 2>err5
-
-printf "timeout: sending signal INT to command 'sleep'\n" > exp-err5 \
-    || framework_failure_
-
-compare exp-err5 err5 || fail=1
-
+# Kill 'sleep' after 1 second with SIGINT - it should terminate
+# (as SIGINT's default action is to terminate a program).
+cat <<\EOF >exp || framework_failure_
+timeout: sending signal INT to command 'env'
+EOF
+env_opt='' retry_delay_ env_ignore_delay_ .1 6 || fail=1
 
 # env test - ignore signal handler
 # --------------------------------
-# Use env to silence (ignore) SIGINT - "seq" should continue running
+# Use env to silence (ignore) SIGINT - "sleep" should continue running
 # after timeout sends SIGINT, and be killed after 1 second using SIGKILL.
-
-cat>exp-err6 <<EOF
+cat <<\EOF >exp || framework_failure_
 timeout: sending signal INT to command 'env'
 timeout: sending signal KILL to command 'env'
 EOF
-
-env timeout --verbose --kill-after=.1 --signal=INT .1 \
-    env --ignore-signal=INT \
-    sleep 10 > /dev/null 2>err6t
-
-# check only the first two lines from stderr, which are printed by timeout.
-# (operating systems might add more messages, like "killed").
-sed -n '1,2p' err6t > err6 || framework_failure_
-
-compare exp-err6 err6 || fail=1
-
+env_opt='--ignore-signal=INT' retry_delay_ env_ignore_delay_ .1 6 || fail=1
 
 # env test - ignore signal handler (2)
 # ------------------------------------
-# Repeat the previous test with "--ignore-signals" and no signal names,
+# Repeat the previous test with "--ignore-signal" and no signal names,
 # i.e., all signals.
-
-env timeout --verbose --kill-after=.1 --signal=INT .1 \
-    env --ignore-signal \
-    sleep 10 > /dev/null 2>err7t
-
-# check only the first two lines from stderr, which are printed by timeout.
-# (operating systems might add more messages, like "killed").
-sed -n '1,2p' err7t > err7 || framework_failure_
-
-compare exp-err6 err7 || fail=1
-
+env_opt='--ignore-signal' retry_delay_ env_ignore_delay_ .1 6 || fail=1
 
 # env test --list-signal-handling
 env --default-signal --ignore-signal=INT --list-signal-handling true \
   2> err8t || fail=1
 sed 's/(.*)/()/' err8t > err8 || framework_failure_
-env printf 'INT        (): IGNORE\n' > exp-err8
+env printf 'INT        (): IGNORE\n' > exp-err8 || framework_failure_
 compare exp-err8 err8 || fail=1

-- 
2.26.2