On 06/03/2013 12:14 PM, Bernhard Voelker wrote: > Hi Padraig, > > thanks again for working on this. > > On 06/03/2013 12:13 PM, Pádraig Brady wrote: >> From 12b6c3bf70dde23c2c1bf5db7650a959f327b04f Mon Sep 17 00:00:00 2001 >> From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> >> Date: Mon, 3 Jun 2013 01:29:17 +0100 >> Subject: [PATCH] tests: avoid a race in tail --retry testing >> >> Prompted by the continuous integration build failure at: >> http://hydra.nixos.org/build/5221053 > > Can we rely on this URL to be available quite some time, or is > there some cleanup on Hydra from time to time? > >> * tests/tail-2/retry.sh: Ensure the 'out' file is not present >> as it's used to arbitrate the run order of commands. >> Relying on the truncation in the background tail command, > > s/,// > >> is racy because the truncation can occur after the fork >> of the background shell and thus the 'missing' file could >> be created by the time tail(1) looks for it. > > I'm not sure I understand the above. > Did you mean > s/missing/out/ > s/tail/wait4lines_/ > ?
As a consequence of wait4lines waiting incorrectly on 'out', the 'missing' file would be populated by the time tail(1) gets to process it. I'll clarify the description. > >> --- >> tests/tail-2/retry.sh | 11 ++++++----- >> 1 files changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/tests/tail-2/retry.sh b/tests/tail-2/retry.sh >> index d56d4c1..070e4fd 100644 >> --- a/tests/tail-2/retry.sh >> +++ b/tests/tail-2/retry.sh >> @@ -44,9 +44,10 @@ grep -F 'tail: warning: --retry ignored' out || fail=1 >> >> # === Test: >> # Ensure that "tail --retry --follow=name" waits for the file to appear. >> +rm out || framework_failure_ >> timeout 10 tail -s.1 --follow=name --retry missing >out 2>&1 & pid=$! > > It doesn't look obvious that rm(1) avoids a race, and someone could > easily remove that 'redundant' line again in a future change. > Would you mind to add a short comment there? You also pointed out that 'out' must be present or the current code in wait4lines would print confusion warnings. So I'll make this additional change: -rm out || framework_failure_ +# Clear 'out' so that we can check its contents without races +:>out || framework_failure_ thanks, Pádraig.
