Re: Make the git codebase thread-safe

Am 14.02.2014 00:09, schrieb Zachary Turner:

To elaborate a little bit more, you can verify with a sample program
that ReadFile with OVERLAPPED does in fact modify the HANDLE's file
position.  The documentation doesn't actually state one way or
another.   My original attempt at a patch didn't have the ReOpenFile,
and we experienced regular read corruption.  We scratched our heads
over it for a bit, and then hypothesized that someone must be mixing
read styles, which led to this ReOpenFile workaround, which
incidentally also solved the corruption problems.  We wrote a similar
sample program to verify that when using ReOpenHandle, and changing
the file pointer of the duplicated handle, that the file pointer of
the original handle is not modified.

We did not actually try to identify the source of the mixed read
styles, but it seems like the only possible explanation.

On Thu, Feb 13, 2014 at 2:53 PM, Stefan Zager [off-list ref] wrote:

quoted

On Thu, Feb 13, 2014 at 2:51 PM, Karsten Blees [off-list ref] wrote:

quoted

Am 13.02.2014 19:38, schrieb Zachary Turner:

quoted

The only reason ReOpenFile is necessary at
all is because some code somewhere is mixing read-styles against the same
fd.

I don't understand...ReadFile with OVERLAPPED parameter doesn't modify the HANDLE's file position, so you should be able to mix read()/pread() however you like (as long as read() is only called from one thread).

That is, apparently, a bald-faced lie in the ReadFile API doc.  First
implementation didn't use ReOpenFile, and it crashed all over the
place.  ReOpenFile fixed it.

Stefan

Damn...you're right, multi-threaded git-index-pack works fine, but some tests fail badly. Mixed reads would have to be from git_mmap, which is the only other caller of pread().

A simple alternative to ReOpenHandle is to reset the file pointer to its original position, as in compat/pread.c::git_pread. Thus single-theaded code can mix read()/pread() at will, but multi-threaded code has to use pread() exclusively (which is usually the case anyway). A main thread using read() and background threads using pread() (which is technically allowed by POSIX) will fail with this solution.

This version passes the test suite on msysgit:

----8<----
ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)
{
	DWORD bytes_read;
	OVERLAPPED overlapped;
	off64_t current;
	memset(&overlapped, 0, sizeof(overlapped));
	overlapped.Offset = (DWORD) offset;
	overlapped.OffsetHigh = (DWORD) (offset >> 32);

	current = lseek64(fd, 0, SEEK_CUR);

	if (!ReadFile((HANDLE)_get_osfhandle(fd), buf, count, &bytes_read, &overlapped)) {
		errno = err_win_to_posix(GetLastError());
		return -1;
	}

	lseek64(fd, current, SEEK_SET);

	return (ssize_t) bytes_read;
}