Re: [PATCH] die routine: change recursion limit from 1 to 1024

On Tue, Jun 20 2017, Jeff King jotted:

If I understand you correctly this on top:

    diff --git a/usage.c b/usage.c
    index 1c198d4882..f6d5af2bb4 100644
    --- a/usage.c
    +++ b/usage.c
    @@ -46,7 +46,19 @@ static int die_is_recursing_builtin(void)
     	static int dying;
     	static int recursion_limit = 1024;

    -	return dying++ > recursion_limit;
    +	dying++;
    +
    +	if (!dying) {
    +		/* ok, normal */
    +		return 0;
    +	} else if (dying < recursion_limit) {
    +		/* only show the warning once */
    +		if (dying == 1)
    +			warning("die() called many times. Recursion error or racy threaded death!");
    +		return 0; /* don't bail yet */
    +	} else {
    +		return 1;
    +	}
     }

     /* If we are in a dlopen()ed .so write to a global variable would segfault

Will yield this over 1000 runs, i.e. mostly this works and we emit the warning (although sometimes we miss it, and we might even emit it twice or more due to an extra race condition we have now):

    $ (for i in {1..1000}; do ~/g/git/git-grep -P --threads=10 '(*LIMIT_RECURSION=1)(*LIMIT_MATCH=1)-?-?-?---$' 2>&1|perl -pe 's/^fatal: r.*/R/; s/^fatal: p.*/P/; s/^warning.*/W/' | tr '\n' ' '; echo; done)|sort|uniq -c|sort -nr|head -n 20
        245 W P P
        222 W P P P
        212 W P
         47 P W P
         36 W P P P P
         35 P P P
         35 P P
         30 P W P P
         16 P P W
         14 W W P P
         12 P P W P
         11 W W P P P
         11 P
          8 W P W P P
          8 P P P P
          7 W P P P P P
          7 P W P P P
          6 W P W P
          4 P W P W
          3 W W P P W

I think it makes sense to apply that on top, even though we could print more than 1 warning here it makes sense to alert the user that we're in the middle of some racy death, it explains the multiple lines of output they'll probably (but not always!) get.

As you can see the third most common case is that we needlessly print out the warning, i.e. we have only one error anyway, but we can't guarantee that, so it probably makes sense to emit it.

To reply to your 20170620161514.ygbflanx4pldc7n7@sigill.intra.peff.net downthread here (where you talk about setting up a custom die handler for grep) yeah that would make sense, but as long as we're supplying this default behavior (and not outlawing using it with pthreads) it makes sense to get out of our own way with this recursion detection.

I think my patch (possibly with the fixup above, depending on what we think about dupe warnings) is just fine to fix this. This is already a super-rare edge case in grep, and to the extent that it would be a problem for anyone it's because our paranoid recursion detector totally hides the error, I don't think it's worth worrying about us printing a few dupe error messages under threading for something that almost never happens.

At least, that's starting to go beyond my bothering to hack on it :)