Re: [PATCH v2] die(): stop hiding errors due to overzealous recursion guard

[Jeff King <peff@peff.net>]

On Wed, Jun 21, 2017 at 02:32:16PM -0700, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
> > So let's just set the recursion limit to a number higher than the
> > number of threads we're ever likely to spawn. Now we won't lose
> > errors, and if we have a recursing die handler we'll still die within
> > microseconds.
> >
> > There are race conditions in this code itself, in particular the
> > "dying" variable is not thread mutexed, so we e.g. won't be dying at
> > exactly 1024, or for that matter even be able to accurately test
> > "dying == 2", see the cases where we print out more than one "W"
> > above.
> 
> One case I'd be worried about would be that the race is so bad that
> die-is-recursing-builtin never returns 0 even once.  Everybody will
> just say "recursing" and die, without giving any useful information.

I was trying to think how that would happen. If nobody's actually
recursing indefinitely, then the value in theory peaks at the number of
threads (modulo the fact that we're modifying a variable from multiple
threads without any locking; I'm not sure how reasonable it is to assume
in practice that sheared writes may cause us to lose an increment but
not to put nonsense in to the variable). If they are, then one thread
may increment it to 1024 before another thread gets a chance to say
anything. But in that case, the recursion-die is our expected outcome.

Anyway, it might be reasonable to protect the counter with a mutex.
Like:

diff --git a/usage.c b/usage.c
index fc2b31c54b..34fef0f9fa 100644
--- a/usage.c
+++ b/usage.c
@@ -44,9 +44,19 @@ static void warn_builtin(const char *warn, va_list params)
 	vreportf("warning: ", warn, params);
 }
 
+#ifndef NO_PTHREADS
+static pthread_mutex_t recursion_mutex = PTHREAD_MUTEX_INITIALIZER;
+#define recursion_lock() pthread_mutex_lock(&recursion_mutex)
+#define recursion_unlock() pthread_mutex_unlock(&recursion_mutex)
+#else
+#define recursion_lock()
+#define recursion_unlock()
+#endif
+static int recursion_counter;
+
 static int die_is_recursing_builtin(void)
 {
-	static int dying;
+	int dying;
 	/*
 	 * Just an arbitrary number X where "a < x < b" where "a" is
 	 * "maximum number of pthreads we'll ever plausibly spawn" and
@@ -55,7 +65,10 @@ static int die_is_recursing_builtin(void)
 	 */
 	static const int recursion_limit = 1024;
 
-	dying++;
+	recursion_lock();
+	dying = ++recursion_counter;
+	recursion_unlock();
+
 	if (dying > recursion_limit) {
 		return 1;
 	} else if (dying == 2) {

I can't remember if there are problems on Windows with using constant
mutex initializers, though. If so, I guess common-main would have to
initialize it.

I left the rest of the logic as-is, but if we switched to post-increment:

  dying = recursion_counter++;

then I think the numbers around "dying" would make more sense (e.g.,
"dying == 2" would make more sense to me as "dying == 1" to check that
we were already dying).

To be honest, I'm not sure if it's worth giving it much more time,
though. I'd be fine with Ævar's patch as-is.

-Peff