From: Shawn O. Pearce Date: Sat, 06 Mar 2010 21:50:51 GMT Subject: Re: [PATCH 0/6] Pass t5530 on Windows Message-ID: <20100306215051.GE2529@spearce.org> In-Reply-To: <7vk4tpdx9x.fsf@alter.siamese.dyndns.org> Junio C Hamano wrote: > Johannes Sixt 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.