patcht6026-merge-attr: don't fail if sleep exits early

Commit 5babb5bdb3 ("t6026-merge-attr: clean up background process at end of test case") added a kill command to clean up after the test, but this can fail if the sleep command exits before the cleanup is executed. Ignore the error from the kill command.

Signed-off-by: Andreas Schwab <schwab@suse.de>
---
The failure can be simulated by adding a sleep after the last command to
delay the cleanup.
---
 t/t6026-merge-attr.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Tue, Nov 08, 2016 at 06:03:04PM +0100, Andreas Schwab wrote:

Thanks for the reproduction hint. I sometimes run the test suite through a "stress" script that sees if a test script racily fails under load, but I wasn't able to trigger it here (I guess a full second is just too long even under high load). But the extra sleep makes it obvious.

Looks like the original is in v2.10.1, so this should probably go to maint, as well as the upcoming v2.11-rc1.

-Peff

Hi 

On Tue, 8 Nov 2016, Jeff King wrote:

The reason why we do not ignore kill errors is that we want to make sure that the script *actually ran*. Otherwise, the thing we need to test here does not necessarily get tested.

So I would rather go with the patch Hannes hinted at when he said that he did not want to change the file name (as it would have made the diff less obvious): increase the number of seconds.

Will send out a superseding patch in a minute, Dscho

On Nov 09 2016, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

That can be tested by looking for the pid file.

Andreas.

On Wed, Nov 09, 2016 at 03:36:40PM +0100, Andreas Schwab wrote:

I agree that makes the intent a lot more obvious. Having a necessary condition of the test stuffed into a test_when_finished block seems counter-intuitive.

-Peff

Commit 5babb5bdb3 ("t6026-merge-attr: clean up background process at end of test case") added a kill command to clean up after the test, but this can fail if the sleep command exits before the cleanup is executed. Ignore the error from the kill command.

Explicitly check for the existence of the pid file to test that the merge driver was actually called.

Signed-off-by: Andreas Schwab <schwab@suse.de>
---
 t/t6026-merge-attr.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Andreas Schwab <schwab@suse.de> writes:

OK. sleep.pid is a reasonable easy-to-access side effect we can observe to make sure that the sleep-one-second merge driver was indeed invoked, which was missing from the earlier round.

Thanks, will apply.

Hi all,

On Thu, 10 Nov 2016, Junio C Hamano wrote:

No, this is incorrect. The condition that we need to know applies is that the script is still running, and blocking if the bug reappears.

It is not enough to test that the script *started* running. If it exits too early, the problematic condition is never tested, and the test is useless.

Ciao, Johannes

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

OK, I see what you are saying, and I see a few things wrong in here:

 * First, the test is titled in a misleading way.  In the context of
   a patch that was titled ad65f7e3b7 ("t6026-merge-attr: child
   processes must not inherit index.lock handles", 2016-08-18), it
   might have been clear enough to say "does not lock index", but
   the sleeping is to make sure that we would notice if the fd to
   the index.lock leaked to the child process by mistake, and the
   way to do so is that the child arranges the leaked fd to be kept
   open after it exits (by spawning "sleep").  The test was never
   about "does not lock index" (the driver does not take any lock by
   itself in the first place).

 * There are three possible outcome from this test:

   - 'git merge' fails.

     This is expected to happen only on Windows and if the code gets
     broken and starts leaking the fd.

   - 'git merge' finishes correctly, the sleep is still running when
     test_when_finished goes to cull it.

     In this case, we KNOW there wasn't any fd leak IF we are on
     Windows where a leaked FD would not allow 'git merge' to
     succeed.  But on other platforms, fd leak that may cause
     trouble for Windows friends will not be caught.

   - 'git merge' finishes correctly, the sleep is no longer
     running because the machine was heavily loaded; a workaround is
     to tolerate failure of culling it.

     In this case, we cannot tell anything from the test.  Even if
     the fd was leaked, 'git merge' may have succeeded even on
     Windows.

As everybody knows there is no appropriate timeout value that is good for everybody. I wonder if we can replace the sleep 1 with something like

	( while sleep 3600; do :; done ) &

so that leaked fd will be kept even in any heavily loaded environment instead?

On Thu, Nov 10, 2016 at 02:30:36PM -0800, Junio C Hamano wrote:

I think you may have missed:

  http://public-inbox.org/git/16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schindelin@gmx.de/

which does roughly that. It does not loop, but I suspect 3600 is plenty in practice.

I do think the test would be a lot more obvious if it confirmed at the end of the test that the process was still running, as opposed to relying on test_when_finished to check it.

-Peff

Jeff King <peff@peff.net> writes:

I agree that "check that the process is still running" is a wrong thing to do in the first place. What would it mean if the process is no longer running? It is a timing-dependent bug in the test; after all we failed to produce the condition that could trigger a bug that we are guarding against. And "let's do '|| :'" is sweeping the bug (not in the code we are testing, but in the test that will fail to notice a bug we are preparing against) under the rug. So I agree with Dscho that we should do that first.

If we ensure that the process is still running, then such a check is a good belt-and-suspenders way to catch a breakage in the mechanism we choose to ensure it. So probably we can require that the kill in the "when finished" part to actually send a signal to a process that is still running.

Is there an equivalent to pause(2) available to shell scripts? I really hate a single "sleep 3600" or anything with a magic number.

On Thu, Nov 10, 2016 at 02:55:06PM -0800, Junio C Hamano wrote:

I think it is usually spelled "read <some-fifo", but we can't use FIFOs here because Windows doesn't have them. You could probably do something with "read <&9" and set up descriptor 9 in the test code. But frankly, that gets complex pretty quickly, as you have to background things.

This minor issue isn't worth it. Just bumping the sleep to 3600 makes the raciness problem go away, and everything works in practice. That's probably good enough for our purposes.

-Peff