Re: [PATCH] index-pack: spawn threads atomically

[Taylor Blau <me@ttaylorr.com>]

On Fri, Jan 05, 2024 at 03:50:34AM -0500, Jeff King wrote:
> The t5309 script triggers a racy false positive with SANITIZE=leak on a
> multi-core system. Running with "--stress --run=6" usually fails within
> 10 seconds or so for me, complaining with something like:
>
>     + git index-pack --fix-thin --stdin
>     fatal: REF_DELTA at offset 46 already resolved (duplicate base 01d7713666f4de822776c7622c10f1b07de280dc?)
>
>     =================================================================
>     ==3904583==ERROR: LeakSanitizer: detected memory leaks
>
>     Direct leak of 32 byte(s) in 1 object(s) allocated from:
>         #0 0x7fa790d01986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
>         #1 0x7fa790add769 in __pthread_getattr_np nptl/pthread_getattr_np.c:180
>         #2 0x7fa790d117c5 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150
>         #3 0x7fa790d11957 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:598
>         #4 0x7fa790d03fe8 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:51
>         #5 0x7fa790d013fd in __lsan_thread_start_func ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:440
>         #6 0x7fa790adc3eb in start_thread nptl/pthread_create.c:444
>         #7 0x7fa790b5ca5b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
>
>     SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).
>     Aborted

We discussed this in another thread (beginning here [1]), and I would be
fine with this approach. I share your feeling that it is a little gross
to have to work around LSan's implementation by tweaking production
code, but I think that this doesn't have to be the most pristine patch
ever written, either ;-).

Just playing devil's advocate for a moment, I wonder if another approach
might be to disable the threading altogether for the purposes of this
test. The performance difference is negligible, and I don't think we're
exercising any interesting paths in this particular test that have to do
with pack.threads > 1 that aren't covered extensively elsewhere.

So, in other words, I think a reasonable approach would be to do
something like:

--- 8< ---
diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh
index 4e910c5b9d..1d132b6324 100755
--- a/t/t5309-pack-delta-cycles.sh
+++ b/t/t5309-pack-delta-cycles.sh
@@ -73,7 +73,7 @@ test_expect_success 'failover to a duplicate object in the same pack' '
 		pack_obj $A
 	} >recoverable.pack &&
 	pack_trailer recoverable.pack &&
-	test_must_fail git index-pack --fix-thin --stdin <recoverable.pack
+	test_must_fail git index-pack --threads=1 --fix-thin --stdin <recoverable.pack
 '

 test_done
--- >8 ---

And call it a day. I built with SANITIZE=leak, and then ran:

    $ GIT_TEST_PASSING_SANITIZE_LEAK=true ./t5309-pack-delta-cycles.sh --stress --run=6

For a while and didn't see any failures. That could be luck, of course,
but without the above patch I was seeing failures within a few seconds.
I'm reasonably confident that this would do the trick.

For what it's worth, I'm fine with either approach, mostly to avoid
tying up more of the list's time discussing the options. But I have a
vague preference towards `--threads=1` since it doesn't require us to
touch production code.

> Rescuing this from:
>
>   https://lore.kernel.org/git/20231221105124.GD570888@coredump.intra.peff.net/
>
> where it was buried deep in a thread. I still think it's kind of gross,
> but it may be the least-bad thing.

In either case, thanks for digging it back up :-).

Thanks,
Taylor

[1]: https://lore.kernel.org/git/xmqqbkasnxba.fsf@gitster.g/