Re: Make the git codebase thread-safe

[Karsten Blees <karsten.blees@gmail.com>]

Am 14.02.2014 20:16, schrieb Zachary Turner:
> For the mixed read, we wouldn't be looking for another caller of
> pread() (since it doesn't care what the file pointer is), but instead
> a caller of read() or lseek() (since those do depend on the current
> file pointer).  In index-pack.c, I see two possible culprits:
> 
> 1) A call to xread() from inside fill()
> 2) A call to lseek in parse_pack_objects()
> 
> Do you think these could be related?  If so, maybe that opens up some
> other solutions?
> 

Yeah, I think that's it. The problem is that the single-threaded part (parse_pack_objects/parse_pack_header) _also_ calls pread (via sha1_object -> get_data_from_pack -> unpack_data). So a pread() that modifies the file position would naturally be bad in this single-threaded scenario. Incidentally, that's exactly what the lstat64 in the version below fixes (similar to git_pread).

> BTW, the version you posted isn't thread safe.

It is true that, in a multi-threaded scenario, my version modifies the file position in some indeterministic way. However, as you noted above, the file position is irrelevant to pread(), so that's perfectly thread-safe, as long as all threads use pread() exclusively.

Using [x]read() in one of the threads would _not_ be thread-safe, but we're not doing that here. Both fill()/xread() and parse_pack_objects()/lseek() are unreachable from threaded_second_pass(), and the main thread just waits for the background threads to complete...

>>> 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;
>>> }
>>>
>>