Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Ramsay,

On Tue, 9 Nov 2021, Ramsay Jones wrote:

> On 08/11/2021 23:59, Johannes Schindelin wrote:
> [snip]
> > I had a look at this and could reproduce... partially. I only managed to
> > make it fail every once in a while.
> >
> > Digging deeper, it turns out that the `lstat()` call in
> > `ipc_get_active_state()` does not receive an `st_mode` indicating a
> > socket, but rather a file (in my tests, it was usually 0100644, but
> > sometimes even 0100755).
> >
> > The reason is, of course, that Cygwin _emulates_ Unix sockets. What is in
> > the file system is just a special file, it is marked with the `system` bit
> > (which only exists on Windows), and its contents start with the tell-tale
> > `!<socket>`.
> >
> > And as you might have guessed, there is a race going on between Cygwin
> > writing that file _and_ flipping that `system` bit, and Git trying to
> > access the Unix socket and encountering an unexpected file.
> >
> > Now, why this only happens in your 32-bit setup, I have no idea.
> >
> > In my tests, the following patch works around the issue. Could I ask you
> > to test it in your environment?
>
> Just FYI, I just tried the patch below (on 64-bit cygwin) and this test
> now works fine for me. (well, run 5 times by hand - not with --stress).

Very good!

I fear that it is a bit late in the -rc cycle to try to get this into the
official v2.34.0. Adam, since you are the maintainer of the Cygwin git
package, would you mind incorporating this patch into Cygwin's version of
Git?

> This is on windows 10 21H1 and cygwin:
>
>   $ uname -a
>   CYGWIN_NT-10.0 satellite 3.3.2(0.341/5/3) 2021-11-08 16:55 x86_64 Cygwin
>   $
>
> [Yes, I updated last night!]

Good thing, too: v3.3.2 fixes a critical bug in the pipe code. One symptom
was that you could not use Git Credential Manager Core as credential
helper (because Git thought that the helper had hung up, well before the
helper sent any information).

Ciao,
Dscho

>
> ATB,
> Ramsay Jones
>
> > -- snip --
> > diff --git a/compat/simple-ipc/ipc-unix-socket.c
> > b/compat/simple-ipc/ipc-unix-socket.c
> > index 4e28857a0a..1c591b2adf 100644
> > --- a/compat/simple-ipc/ipc-unix-socket.c
> > +++ b/compat/simple-ipc/ipc-unix-socket.c
> > @@ -36,6 +36,23 @@ enum ipc_active_state ipc_get_active_state(const char
> > *path)
> >  	}
> >
> >  	/* also complain if a plain file is in the way */
> > +#ifdef __CYGWIN__
> > +	{
> > +		static const int delay[] = { 1, 10, 20, 40, -1 };
> > +		int i;
> > +
> > +		for (i = 0; S_ISREG(st.st_mode) && delay[i] > 0; i++) {
> > +			/*
> > +			 * Cygwin might still be in the process of marking the
> > +			 * underlying file as a system file.
> > +			 */
> > +			sleep_millisec(delay[i]);
> > +			if (lstat(path, &st) == -1)
> > +				return IPC_STATE__INVALID_PATH;
> > +		}
> > +	}
> > +#endif
> > +
> >  	if ((st.st_mode & S_IFMT) != S_IFSOCK)
> >  		return IPC_STATE__INVALID_PATH;
> >
> > -- snap --
> >
> > FWIW it looks as if the loop might be a bit of an overkill, as I could not
> > get the code to need more than a single one-millisecond sleep, but it's
> > probably safer to just keep the delay loop in place as-is.
> >
> > Ciao,
> > Dscho
> >
>
>