Re: [PATCH v2] die(): stop hiding errors due to overzealous recursion guard
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 21, 2017, 21:32 UTC
- Message-ID
- <xmqq1sqdni1r.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170621204742.15722-1-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 10 quoted lines
> 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.
Will queue, as it is nevertheless an improvement over the current code.
Thanks.
Show 32 quoted lines
> usage.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/usage.c b/usage.c
> index 2f87ca69a8..1ea7df9a20 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -44,7 +44,23 @@ static void warn_builtin(const char *warn, va_list params)
> static int die_is_recursing_builtin(void)
> {
> static int dying;
> - return dying++;
> + /*
> + * Just an arbitrary number X where "a < x < b" where "a" is
> + * "maximum number of pthreads we'll ever plausibly spawn" and
> + * "b" is "something less than Inf", since the point is to
> + * prevent infinite recursion.
> + */
> + static const int recursion_limit = 1024;
> +
> + dying++;
> + if (dying > recursion_limit) {
> + return 1;
> + } else if (dying == 2) {
> + warning("die() called many times. Recursion error or racy threaded death!");
> + return 0;
> + } else {
> + return 0;
> + }
> }
>
> /* If we are in a dlopen()ed .so write to a global variable would segfault