On Tue, Jun 20 2017, Jeff King jotted:
> On Mon, Jun 19, 2017 at 10:00:36PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> Change the recursion limit for the default die routine from a *very*
>> low 1 to 1024. This ensures that infinite recursions are broken, but
>> doesn't lose error messages.
>>
>> The intent of the existing code, as explained in commit
>> cd163d4b4e ("usage.c: detect recursion in die routines and bail out
>> immediately", 2012-11-14), is to break infinite recursion in cases
>> where the die routine itself dies.
>
> I agree that was the original intent, but I think it also does something
> else. Anytime die() recurses, even a single level, we're going to cover
> up the original failure with the one that happened inside die(), which
> is almost certainly the less interesting of the two.
>
> E.g., if I
>
> die_errno("unable to open %s", filename);
>
> and then the die handler calls malloc() and fails, you'd much rather see
> that first message than "out of memory".
>
> To be fair, "die handler is recursing" is _also_ not helpful, but at
> least it's clear that this is a bug (and IMHO it should be marked with
> BUG()). Saying "out of memory" tells you about the second error, but it
> doesn't tell you that we've masked the first error. So it may lead to
> more confusion in the long run.
>
> I wonder if we can get the best of both, though. Can we make the logic
> more like:
>
> if (!dying) {
> /* ok, normal */
> return 0;
> } else if (dying < 1024) {
> /* only show the warning once */
> if (dying == 1)
> warning("I heard you liked errors, so I put a die() in your die()");
> return 0; /* don't bail yet */
> } else {
> BUG("recursion detected in die handler");
> }
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@xxxxxxxxxxxxxxxxxxxxx
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 :)
>> Now, git-grep could make use of the pluggable error facility added in
>> commit c19a490e37 ("usage: allow pluggable die-recursion checks",
>> 2013-04-16).
>
> Yeah, I think this is a bug in git-grep and should be fixed, independent
> of this commit. You should be able to use as a template the callbacks
> added by the child of c19a490e37:
>
> 1ece66bc9 (run-command: use thread-aware die_is_recursing routine,
> 2013-04-16)
>
> -Peff
