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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Mar 6, 2010, 21:50 UTC
Message-ID: <20100306215051.GE2529@spearce.org>
In-Reply-To: <7vk4tpdx9x.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> 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...

...

Show 6 quoted lines

>    . 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.