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

[PATCH 0/5] fixing thread races in linux-leaks CI

From
Jeff King <peff@peff.net>
Date
Dec 30, 2024, 04:23 UTC
Message-ID
<20241230042325.GA112439@coredump.intra.peff.net>

This series should fix the races we see in linux-leaks CI jobs. Or at least some of them. We tried previously in 993d38a066 (index-pack: spawn threads atomically, 2024-01-05), but it wasn't enough. This series covers index-pack as well as git-grep, which are the two I've seen in practice. We may run into more, but the general pattern should be applicable if we do.

This series takes the minimal approach, and only provides a knob to use pthread_barrier_t if your system supports it (and then turns it on for our CI jobs). The obvious alternative is to actually implement wrappers for other systems, and then use it everywhere. That doesn't buy us much for this problem, but it's possible we'd find a use for barriers elsewhere.

The even farther alternatives are:
  1. Do nothing. This is arguably an LSan bug, which should be doing its
     own locking to synchronize the at-exit leak check with the
     per-thread setup code. I don't think we've even reported it there,
     so one option would be to work with them. Even if we do that, it
     might be nice to remove the false positive pain in the meantime
     with a series like this (and certainly patches 1 and 2 here are
     worth applying independently).
  2. Change our posture on thread exit. In index-pack, the issue is that
     one worker thread calls exit() via die(), taking down the whole
     program (including other worker threads in unknown states). If it
     installed a die() handler that called pthread_exit() instead, then
     the main thread could see that failure and cancel/join the
     remaining threads.
     But I suspect it's not so simple:
       a. We still want the error in the worker thread to cancel ongoing
	  work immediately. Right now index-pack just called
	  pthread_join() in sequence to wait for all threads to finish.
	  But instead we'd need to use some synchronization primitives
	  to report the error. Probably not too hard, but new
	  potentially tricky code.
       b. In the git-grep case, it's actually the main thread that calls
	  die(). So the rule is not really "threads should install a die
	  handler that calls pthread_exit()", but _any_ die() call when
	  threads are established would need to cleanly ask all threads
	  to stop (whether by signaling them via the work queue, or just
	  hitting them with pthread_cancel; and that's assuming that
	  there's no race between LSan's setup code and cancel).
       c. When we call die(), all bets are off about what state various
          data structures are in. For example, a thread could be holding
	  a lock, and if it's cancelled by another thread, that lock
	  will remain. So it's not safe to do any real work in the other
	  threads, unless we start setting up pthread_cleanup handlers,
	  etc. That seems like a recipe for obscure, racy deadlocks.
     I think that may be an appealing direction in the long run,
     especially since die() itself is not thread-safe. But coupled with
     libification, we probably want less "threads can die() cleanly" and
     more "threads pass errors up the stack and report the problem back
     to the work queue". Either way, though, it's a much bigger approach
     change than I think we want just to try to address LSan races.

So I think we want something like this (either this, or the variant where we really implement barriers everywhere) in the near-term.

  [1/5]: test-lib: use individual lsan dir for --stress runs
  [2/5]: Revert "index-pack: spawn threads atomically"
  [3/5]: thread-utils: introduce optional barrier type
  [4/5]: index-pack: work around LSan threading race with barrier
  [5/5]: grep: work around LSan threading race with barrier
 Makefile             |  7 +++++++
 builtin/grep.c       |  8 ++++++++
 builtin/index-pack.c |  8 ++++++--
 ci/lib.sh            |  1 +
 t/test-lib.sh        |  2 +-
 thread-utils.h       | 17 +++++++++++++++++
 6 files changed, 40 insertions(+), 3 deletions(-)
Next: Jeff King
Message 1 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.