Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Mon, Dec 30, 2024 at 09:33:20AM -0800, Junio C Hamano wrote:
>
>> * jk/lsan-race-with-barrier (2024-12-30) 5 commits
> ...
> This graduated faster than I expected. :)

Heh, it is before -rc2 and the change is only about tests, so ...

> ...
> So that line is doing something useful. But it may not be worth the racy
> pain it's causing. So some alternatives are:
>
>   - we drop that line by default, and then when people are investigating
>     a specific leak, they can override LSAN_OPTIONS themselves to get
>     better output (though of course knowing that you can even do is
>     tricky)
>
>   - we keep that line by default, but override LSAN_OPTIONS for CI to
>     avoid the race. That makes all local leak-checking traces
>     informative by default. But CI ones may be truncated. I'm not sure
>     if people use the CI ones directly, or investigate further
>     themselves.
>
>   - we could annotate individual scripts or even tests to disable the
>     option (since it's really just threaded programs). This is more
>     hassle, but would limit the blast radius.
>
> I don't love any of those, but they may be less bad than all of the
> barrier trickery. And it may be that this is even something we could get
> fixed in LSan upstream, and it would just be a temporary workaround. I'm
> still going to pursue that.
>
> And finally, one other option (that I'm not sure why I didn't consider
> before): can we just ignore the false positives, similar to what we did
> in 370ef7e40d (test-lib: ignore uninteresting LSan output, 2023-08-28).

Good point.

> I think we'd have to stop doing abort_on_error for the leak checker and
> just rely on the logs, but that's OK (we always check the logs these
> days).
> ...
> A little hacky, but it lets us have our cake and eat it, too. No changes
> to the code, and no bad stack traces.
>
> What do you think?

I like the small hack.  "This is ultimately LSan's racy-ness and not
ours, so let's avoid changing our code to work it around when we can
do the workaround somewhere else" is an attitude that I would endorse
fully.

Thanks.