git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/5] Revert "index-pack: spawn threads atomically"

From
Jeff King <peff@peff.net>
Date
Dec 30, 2024, 04:26 UTC
Message-ID
<20241230042610.GB113400@coredump.intra.peff.net>
In-Reply-To
<20241230042325.GA112439@coredump.intra.peff.net>
This reverts commit 993d38a0669a8056d496797516e743e26b6b8b54.

That commit was trying to solve a race between LSan setting up the threads stack and another thread calling exit(), by making sure that all pthread_create() calls have finished before doing any work that might trigger the exit().

But that isn't sufficient. The setup code actually runs in the individual threads themselves, not in the spawning thread's call to pthread_create(). So while it may have improved the race a bit, you can still trigger it pretty quickly with:

  make SANITIZE=leak
  cd t
  ./t5309-pack-delta-cycles.sh --stress
Let's back out that failed attempt so we can try again.
Signed-off-by: Jeff King <peff@peff.net>
---
I'm still puzzled why I ever thought 93d38a066 solved this, because the
stress test above is exactly what I was using back then to test it.
Possibly LSan changed in some way, or possibly I just got lucky in my
stress run, but the more likely reason is that I simply bungled the
testing (e.g., if you accidentally use a non-LSan build, then it
naturally appears to work).
 builtin/index-pack.c | 2 --
 1 file changed, 2 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index d773809c4c..0b62b2589f 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1336,15 +1336,13 @@ static void resolve_deltas(struct pack_idx_option *opts)
 	base_cache_limit = opts->delta_base_cache_limit * nr_threads;
 	if (nr_threads > 1 || getenv("GIT_FORCE_THREADS")) {
 		init_thread();
-		work_lock();
 		for (i = 0; i < nr_threads; i++) {
 			int ret = pthread_create(&thread_data[i].thread, NULL,
 						 threaded_second_pass, thread_data + i);
 			if (ret)
 				die(_("unable to create thread: %s"),
 				    strerror(ret));
 		}
-		work_unlock();
 		for (i = 0; i < nr_threads; i++)
 			pthread_join(thread_data[i].thread, NULL);
 		cleanup_thread();
-- 
bar
Previous: Junio C HamanoNext: Jeff King
Message 5 of 11 in “fixing thread races in linux-leaks CI”
  1. 0/5 fixing thread races in linux-leaks CIJeff King, Dec 30, 2024
  2. 1/5 test-lib: use individual lsan dir for --stress runsJeff King, Dec 30, 2024
  3. Jeff KingDec 30, 2024
  4. Junio C HamanoDec 30, 2024
  5. 2/5 Revert "index-pack: spawn threads atomically"Jeff King, Dec 30, 2024
  6. 3/5 thread-utils: introduce optional barrier typeJeff King, Dec 30, 2024
  7. Patrick SteinhardtDec 30, 2024
  8. Jeff KingJan 1, 2025
  9. Patrick SteinhardtJan 3, 2025
  10. 4/5 index-pack: work around LSan threading race with barrierJeff King, Dec 30, 2024
  11. 5/5 grep: work around LSan threading race with barrierJeff King, Dec 30, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.