Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available

From: Johannes Sixt <j6t@kdbg.org>
Date: Mar 23, 2010, 20:19 UTC
Message-ID: <201003232119.19430.j6t@kdbg.org>
In-Reply-To: <4c8ef71003230115y64d36094y178fcfe6576e9c66@mail.gmail.com>

On Dienstag, 23. März 2010, Fredrik Kuivinen wrote:

Show 14 quoted lines

> On Wed, Mar 17, 2010 at 22:28, Johannes Sixt <j6t@kdbg.org> wrote:
> > ----------
> > convert.c:filter_buffer()
> ...
> Maybe I'm missing something but, isn't it possible that xrealloc is
> called simultaneously from the two threads if GIT_TRACE is set?
>
> Immediately after start_async the parent calls strbuf_read. We then
> get the call chain
> strbuf_read -> strbuf_grow -> ALLOG_GROW -> xrealloc, so xrealloc is
> called before we read any data in the parent.
>
> In the child we have start_command -> trace_argv_printf -> strbuf_grow ->
> ...

Outch! You are right. It seems I missed the call of strbuf_grow before the loop in strbuf_read.

OK, this means that convert.c is not safe if (and only if) GIT_TRACE is set. :-(

> That xmalloc and xrealloc aren't thread-safe feels a bit fragile.
> Maybe we should try to fix that.

The point of this assessment was to find out whether this is necessary (and whether something else that is not thread-safe is used).

Show 7 quoted lines

> > ----------
> > upload_pack:create_pack_file(): 
> ...
> sha1_to_hex is also called by the parent and the current
> implementation of that function is not thread-safe. sha1_to_hex is
> also called by some paths in the revision machinery, but I don't know
> if it will ever be called in this particular case.

sha1_to_hex is only called by the parent when the async procedure is not used.

-- Hannes