Re: [PATCH 0/6] Pass t5530 on Windows

["Shawn O. Pearce" <spearce@spearce.org>]

Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > Quite frankly, I don't quite know what to do with this series. On the
> > one hand, it is a clean-up, but in practice it is not relevant whether
> > die() kills only the async thread or the whole process because all
> > callers of async die() themselves anyway when the async procedure died.
> > On the other hand, it does enable threaded async procedures on POSIX...
...
>    . It must not change the program's state that the caller of the
>      facility also uses.
> 
> And calling die() from async is obviously "change the program's state that
> the caller of the facility also uses".  We didn't uncover this as a bug
> because the above "serious restrictions" go both ways.

I agree with you Junio.  I think that any async helper that is
invoking die() in its code path is wrong.  Just like its also
wrong for an async helper to try and use the sha1_file.c family
of functions.  The helper should return failure and let its caller
handle the process termination.

Hell, its even wrong for an async helper to use xmalloc(), because in
a low-memory situation that xmalloc may try to remove pack windows
*without locking*.  _DOUBLE_PLUS_UNGOOD_
 
> If we make threaded-async the default on any platform that is thread
> capable, we would increase the likelihood of catching bugs that violate
> the latter condition.

I'm in favor of that.  If we have threaded delta search enabled,
we probably can also run these async procedures in a POSIX thread
rather than forking off a child.  Though on our primary target
platform of Linux, the performance difference either way probably
cannot be measured.

-- 
Shawn.