[PATCH v2 0/2] avoid bogus "recursion detected in die handler" message

[Jeff King <peff@peff.net>]

On Tue, Apr 16, 2013 at 04:13:56PM +0200, Johannes Sixt wrote:

> > I'm not clear on what you are suggesting. That we protect only the main
> > thread from recursion, or that we drop the check entirely? Or that we
> > implement thread-local storage for this case without using pthread_once?
> 
> Anything(*) that does not require pthread_once. A pthread_once
> implementation on Windows would be tricky and voluminous and and on top of
> it very likely to be done differently for gcc and MSVC. I don't like to go
> there if we can avoid it.

We don't need to use pthread_once, as we can just do the initialization
of the thread-local storage along with starting the first thread (where
we already do similar initialization).  Patch series to follow:

  [1/2]: usage: allow pluggable die-recursion checks
  [2/2]: run-command: use thread-aware die_is_recursing routine

> (*) That includes doing nothing, but does not include ripping out the
> recursion check, as it protects us from crashes.

I don't think doing nothing is a good idea. The recursion-detection is
triggering erroneously, blocking real error messages and replacing them
with the scary red-herring "recursion detected in die handler".

The absolute simplest thing I think we could do is basically:

diff --git a/run-command.c b/run-command.c
index 765c2ce..3b0ad44 100644
--- a/run-command.c
+++ b/run-command.c
@@ -599,11 +599,14 @@ static NORETURN void die_async(const char *err, va_list params)
 	return (void *)ret;
 }
 
+extern int dying;
+
 static NORETURN void die_async(const char *err, va_list params)
 {
 	vreportf("fatal: ", err, params);
 
 	if (!pthread_equal(main_thread, pthread_self())) {
+		dying = 0; /* undo counter */
 		struct async *async = pthread_getspecific(async_key);
 		if (async->proc_in >= 0)
 			close(async->proc_in);
diff --git a/usage.c b/usage.c
index 40b3de5..cf8a968 100644
--- a/usage.c
+++ b/usage.c
@@ -6,7 +6,7 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
-static int dying;
+int dying;
 
 void vreportf(const char *prefix, const char *err, va_list params)
 {

Obviously it would help to wrap it in a "clear_die_counter()" function,
but it would still suffer from the problem that there is no
synchronization. In the moment between incrementing and resetting the
dying counter, another thread (including the main program) could check
it. In practice, this does not happen in the current code, because we
do not start many async threads (and we only die in the main thread once
the async thread dies). But it seems unnecessarily flaky and prone to
future problems.

It would also be possible to use mutexes to make it work reliably, but
I'd be very concerned about increasing the complexity of the die code
path.  We would never want a hung thread to prevent the main program
from successfully exiting, for example.

So I think the right solution is just a per-thread counter.

-Peff