Re: Make the git codebase thread-safe
- From
- Zachary Turner <zturner@chromium.org>
- Date
- Feb 15, 2014, 01:15 UTC
- Message-ID
- <CAAErz9gO3NrAF5Zhu277NLqBv-4otQVWGBP6fX00x2OJ3v0_fg@mail.gmail.com>
- In-Reply-To
- <CACsJy8AQNmmW40R-H7kz1dmwiaSKVgu+GP=Jt1qTKgfbZoMkMA@mail.gmail.com>
Even if we make that change to use TLS for this case, the implementation of pread() will still change in such a way that the semantics of pread() are different on Windows. Is that ok?
Just to summarize, here's the viable approaches I've seen discussed so far:
1) Use _WINVER at compile time to select either a thread-safe or non-thread-safe implementation of pread. This is the easiest possible code change, but would necessitate 2 binary distributions of git for windows. 2) Use TLS as you suggest and have one fd per pack thread. Probably the most complicated code change (at least for me, being a first-time contributor) 3) Use Karsten's suggested implementation from earlier in the thread. Seems to work, but it's a little confusing from a readability standpoint since the implementation is not-thread safe except in this specific usage context.
On Fri, Feb 14, 2014 at 4:56 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 29 quoted lines
> On Sat, Feb 15, 2014 at 7:50 AM, Stefan Zager <szager@google.com> wrote: >> On Fri, Feb 14, 2014 at 4:45 PM, Duy Nguyen <pclouds@gmail.com> wrote: >>> On Sat, Feb 15, 2014 at 2:16 AM, Zachary Turner <zturner@chromium.org> wrote: >>>> (Gah, sorry if you're receiving multiple emails to your personal >>>> addresses, I need to get used to manually setting Plain-text mode >>>> every time I send a message). >>>> >>>> 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? >>> >>> For index-pack alone, what's wrong with open one file handle per thread? >> >> Nothing wrong with that, except that it would mean either using >> thread-local storage (which the code doesn't currently use); or >> plumbing pack_fd through the call stack, which doesn't sound very fun. > > Current code does use thread-local storage (struct thread_local and > get_thread_data). Adding a new file handle when NO_THREAD_SAFE_PREAD > is defined is simpler imo. > -- > Duy