From: Johannes Sixt Date: Tue, 23 Mar 2010 20:19:18 GMT Subject: Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available Message-ID: <201003232119.19430.j6t@kdbg.org> In-Reply-To: <4c8ef71003230115y64d36094y178fcfe6576e9c66@mail.gmail.com> On Dienstag, 23. März 2010, Fredrik Kuivinen wrote: > On Wed, Mar 17, 2010 at 22:28, Johannes Sixt 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). > > ---------- > > 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