Re: cat-file timing window on Cygwin

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Aug 26, 2017, 00:57 UTC
Message-ID: <253bf111-4e8a-54b7-6e39-2908270aa357@ramsayjones.plus.com>
In-Reply-To: <20170825150819.agxvbjytom7ao6n6@sigill.intra.peff.net>

On 25/08/17 16:08, Jeff King wrote:

Show 7 quoted lines

> On Fri, Aug 25, 2017 at 12:25:29PM +0100, Adam Dinwoodie wrote:
> 
>> As of v2.10.0-rc1-4-g321459439 ("cat-file: support --textconv/--filters
>> in batch mode"), t8010-cat-file-filters.sh has been failing on Cygwin.
>> Digging into this, the test looks to expose a timing window: it appears
>> that if `git cat-file --textconv --batch` receives input on stdin too
>> quickly, it fails to parse some of that input.

This has not been failing since the above commit (when this feature was first introduced), but (for me) when testing the v2.13.0-rc0 build. Please refer to my original email:

https://public-inbox.org/git/bf7db655-d90f-e711-afc8-6565b71373d2@ramsayjones.plus.com/

... where I was reasonably sure this was caused by a change in the cygwin dll while upgrading from 2.7.0-1 -> 2.8.0-1.

Show 5 quoted lines

> Hmm. That commit doesn't seem to actually touch the stdin loop. That
> happens via strbuf_getline(), which should be pretty robust to timing.
> But here are a few random thoughts:
> 
>   1. Do you build with HAVE_GETDELIM? Does toggling it have any effect?

If Adam builds using the configure script, then yes, else no. ;-) I never use configure, so I don't have HAVE_GETDELIM set.

Show 6 quoted lines

>   2. Does the problem happen only with --textconv?
> 
>      If so, I wonder if spawning the child process is somehow draining
>      stdin. I don't see how the child would accidentally read from our
>      stdin; we replace its stdin with a pipe so it shouldn't get a
>      chance to see our descriptor at all.

As I said in my previous email, "the loop in batch_objects() (builtin/cat-file.c lines #489-505) only reads one line from stdin, then gets EOF on the stream."

Show 5 quoted lines

>      If we accidentally called fflush(stdin) that would discard buffered
>      input and give the results you're seeing. We don't do that
>      directly, of course, but we do call fflush(NULL) in run-command
>      before spawning the child. That _should_ just flush output handles,
>      but it's possible that there's a cygwin bug, I guess.

I suspect so, see previous email ...

Show 15 quoted lines

> I can't reproduce here on Linux. But does the patch below have any
> impact?
> 
> diff --git a/run-command.c b/run-command.c
> index 98621faca8..064ebd1995 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -641,7 +641,6 @@ int start_command(struct child_process *cmd)
>  	}
>  
>  	trace_argv_printf(cmd->argv, "trace: run_command:");
> -	fflush(NULL);
>  
>  #ifndef GIT_WINDOWS_NATIVE
>  {

I suspect not, but I can give it a try ...

... oh, wow, that works! Ahem. (Hmm, so it's flushing stdin?!)

Also, t5526-fetch-submodules.sh still works as well (see commit 13af8cbd6a "start_command: flush buffers in the WIN32 code path as well", 04-02-2011).

ATB, Ramsay Jones