Re: [PATCH] Enable sub-second timeout in zsystem flock

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Cedric Ware wrote on Sat, 14 Mar 2020 22:04 +0100:
> dana (Thursday 2020-03-12):
> > 2. This particular test doesn't seem reliable on my machine. Within the test
> >    harness, it normally takes about 0.078 seconds. Probably the fork over-head
> >    (which is pretty high on macOS) is greater than the amount of time you're
> >    giving it wait? If i change `zselect -t 1` to `zselect -t 10` it seems to
> >    work better... but it still feels very brittle. Very much dependent on the
> >    hardware, the OS, and the current resource utilisation  
> 
> I see.  Here's a new version of the patch.  As long as the tests are
> run sequentially and not in parallel, there shouldn't be any race
> condition left.  Instead of just waiting 10 ms and hoping that the
> sub-shell has had time to start in the background, I'm now actually
> testing the presence of a file that it creates.
> 
> It might still fail if the background sub-shell completes, including
> the several-tenths-of-a-second wait, before the next part of the test
> is run.  Do you think it's still too likely?

> +++ zsh-5.8/Test/V14system.ztst	2020-03-14 21:59:02.351858164 +0100
> @@ -0,0 +1,131 @@
> +# Test zsh/system module
> +  (
> +    # Lock file for 1 second in the background.
> +    (zsystem flock $tst_dir/file \
> +     && touch $tst_dir/locked \
> +     && zselect -t 100
> +     rm -f $tst_dir/locked) &
> +    # Wait until sub-shell above has started.
> +    while ! [[ -f $tst_dir/locked ]]; do
> +      zselect -t 1
> +    done

If «zsystem flock» returns non-zero, this loop will never terminate.

Tests should be written to always terminate, if possible; and if not,
they should warn about that on $ZTST_fd.  (The output of «make check»
has several examples of the latter.)

There are additional instances of this later in the file.

> +    # Attempt to lock file with 0.5 second timeout: must fail.
> +    zsystem flock -t 0.5 $tst_dir/file
> +  )
> +2:zsystem flock unsuccessful wait test
> +
> +  (
> +    # Wait until sub-shell of the previous test has finished.
> +    while [[ -f $tst_dir/locked ]]; do
> +      zselect -t 10
> +    done

Wouldn't it be easier to use a different file in this test than in the
previous test?  Tests should be independent of each other if possible.

> +    # Lock file for 0.5 second in the background.
> +    (zsystem flock $tst_dir/file \
> +      && touch $tst_dir/locked \
> +      && zselect -t 50
> +      rm -f $tst_dir/locked) &
> +    # Wait until sub-shell above has started.
> +    while ! [[ -f $tst_dir/locked ]]; do
> +      zselect -t 1
> +    done
> +    typeset -F SECONDS
> +    start=$SECONDS
> +    # Attempt to lock file without a timeout:
> +    # must succeed after sub-shell above releases it (0.5 second).
> +    if zsystem flock $tst_dir/file; then
> +      elapsed=$[ $SECONDS - $start ]
> +      if [[ $elapsed -ge 0.3 && $elapsed -le 0.7 ]]; then
> +        echo "elapsed time seems OK" 1>&2
> +      else
> +        echo "elapsed time $elapsed should be ~ 0.5 second" 1>&2
> +      fi
> +    fi
> +  )
> +0:zsystem flock successful wait test, no timeout
> +?elapsed time seems OK

How about adding some "F:" lines (to this and subsequent tests)
explaining that failure doesn't necessarily indicate a problem in zsh,
but could also be caused by process scheduling issues?

Cheers,

Daniel