Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Aug 10, 2018, 16:51 UTC
Message-ID: <70b25098-bf96-d362-56cd-2bb17cadf162@jeffhostetler.com>
In-Reply-To: <811ded48-6f33-c46e-7bae-b9f7c7e8764c@kdbg.org>

On 8/10/2018 12:15 PM, Johannes Sixt wrote:

Show 28 quoted lines

> Am 09.08.2018 um 19:35 schrieb Johannes Schindelin via GitGitGadget:
>> I reported a couple of times that t5552 is not passing reliably. It 
>> has now
>> reached next, and will no doubt infect master soon.
>>
>> Turns out that it is not a Windows-specific issue, even if it occurs a 
>> lot
>> more often on Windows than elsewhere.
>>
>> The culprit is that two processes try simultaneously to write to the same
>> file specified via GIT_TRACE_PACKET, and it is not well defined how that
>> should work, even on Linux.
> 
> Thanks for digging down to the root cause. As has been said, the 
> behavior of O_APPEND is well-defined under POSIX, but last time I looked 
> for equivalent feature on Windows, I did not find any.
> 
> Last time was when I worked around the same failure in 
> t5503-tagfollow.sh in my private builds:
> 
> https://github.com/j6t/git/commit/9a447a6844b50b43746d9765b3ac809e2793d742
> 
> It is basically the same as Peff suggests: log only one side of the fetch.
> 
> As this buglet looks like a recurring theme, and a proper fix is 
> preferable over repeated work-arounds. To me it looks like we need some 
> sort of locking on Windows. Unless your friends at Microsoft have an ace 
> in their sleeves that lets us have atomic O_APPEND the POSIX way...

The official Windows CRT for open() does document an O_APPEND, but after looking at the distributed source, I'm not sure I trust it.

I have not looked at the MSYS version of the CRT yet so I cannot comment on it.

I did find that the following code does what we want. Notice the use of FILE_APPEND_DATA in place of the usual GENERIC_READ. (And yes, the docs are very vague here.)

Before we try a locking solution, perhaps we should tweak mingw_open() to use something like this to get a proper HANDLE directly and then use _open_osfhandle() to get a "fd" for it.

Jeff

---------------------------------------------------------------------

#include <Windows.h>

int main()
{
	HANDLE h = CreateFileW(L"foo.txt",
		FILE_APPEND_DATA,
		FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
		NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);

	const char * buf = "this is a test\n";
	DWORD len = strlen(buf);
	DWORD nbw;

	WriteFile(h, buf, len, &nbw, NULL);
	CloseHandle(h);

	return 0;
}