Re: t5400 failure on Windows

From: Jeff King <peff@peff.net>
Date: May 17, 2017, 05:44 UTC
Message-ID: <20170517054424.cq66cl5ukvhsz34y@sigill.intra.peff.net>
In-Reply-To: <afca6bf5-472e-dda4-2c16-a2256080ac51@kdbg.org>

On Tue, May 16, 2017 at 08:35:46PM +0200, Johannes Sixt wrote:

Show 17 quoted lines

> Am 16.05.2017 um 00:24 schrieb Jeff King:
> >    4. There is something racy and unportable about both programs writing
> >       to the same trace file. It's opened with O_APPEND, which means that
> >       each write should atomically position the pointer at the end of the
> >       file. Is it possible there's a problem with that in the way
> >       O_APPEND works on Windows?
> > 
> >       If that was the case, though, I'd generally expect a sheared write
> >       or a partial overwrite. The two origin/HEAD lines from the two
> >       programs are the exact same length, but I'd find it more likely for
> >       the overwrite to happen with one of the follow-on lines.
> 
> Good guesswork! O_APPEND is not atomic on Windows, in particular, it is
> emulated as lseek(SEEK_END) followed by write().
> 
> I ran the test a few more times, and it fails in different ways (different
> missing lines) and also succeeds in a minority of the cases.

OK, that definitely explains it, then.

> Windows is capable of writing atomically in the way O_APPEND requires
> (keyword: FILE_APPEND_DATA), but I do not see a way to wedge this into the
> emulation layer. For the time being, I think I have to skip the test case.

I wonder if there's a way we could convince the trace for the two programs to go to separate locations. We don't care about receive-pack's trace at all. So maybe:

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 3331e0f53..d375d7110 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -288,7 +288,10 @@ test_expect_success 'receive-pack de-dupes .have lines' '
 	$shared .have
 	EOF
 
-	GIT_TRACE_PACKET=$(pwd)/trace git push fork HEAD:foo &&
+	GIT_TRACE_PACKET=$(pwd)/trace \
+	    git push \
+		--receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \
+		fork HEAD:foo &&
 	extract_ref_advertisement <trace >refs &&
 	test_cmp expect refs
 '

> The question remains how endangered our uses of O_APPEND are when the POSIX
> semantics are not obeyed precisely:
> 
> * trace.c: It is about debugging. Not critical.

Agreed.

> * sequencer.c: It writes the "done" file. No concurrent accesses are
> expected: Not critial.

Agreed.

> * refs/files-backend.c: There are uses in functions open_or_create_logfile()
> and log_ref_setup(). Sounds like it is in reflogs. Sounds like this is about
> reflogs. If there are concurrent accesses, there is a danger that a reflog
> is not recorded correctly. Then reflogs may be missing and objects may be
> pruned earlier than expected. That's something to worry about, but I would
> not count it as mission critical.

We should always hold the matching ref lock already when modifying a
reflog. I seem to recall there was a case that missed this (I think
maybe modifying the HEAD reflog without holding HEAD.lock), but it was
fixed in the last few versions.

So I think we're probably OK, but I agree it's an interesting gotcha
that may bite us in the future.

-Peff