Skip to content

larson: fix two intermittent completion hangs#266

Open
hosking wants to merge 1 commit into
daanx:masterfrom
hosking:fix-larson-completion-hangs
Open

larson: fix two intermittent completion hangs#266
hosking wants to merge 1 commit into
daanx:masterfrom
hosking:fix-larson-completion-hangs

Conversation

@hosking

@hosking hosking commented Jul 6, 2026

Copy link
Copy Markdown

runthreads() waits for the worker threads by spinning on each slot's finished flag (while (!de_area[i].finished) sched_yield();). Two bugs can leave a slot's finished false forever, so larson/larson-sized occasionally hang at shutdown instead of finishing.

Both are latent, timing-dependent races: they surface when thread startup/scheduling is slow enough to widen the window (I hit them consistently under a GC-based allocator whose per-thread setup delays a worker past stopflag; the system allocator misses the window by luck). They are allocator-independent bugs in the harness itself.

1. Data race on stopflag / finished. Both are shared between the main thread (sets stopflag, reads finished) and the worker/respawn threads (read stopflag, write finished) as plain volatile int. volatile gives neither atomicity nor inter-thread ordering, so these are unsynchronized data races (UB). ThreadSanitizer flags both (on a standalone build with the system allocator — allocator-independent). Fix: make them std::atomic<int>.

2. Missing completion on the stopflag early-return. A worker that starts after main has set stopflag = TRUE hits if (stopflag) return 0; and returns without setting finished = TRUE, leaving that slot's finished at its initial 0 forever → main spins on it. Fix: mark the slot finished before that early-return.

Verification

  • ThreadSanitizer: the stopflag and finished data races are reported before the change and gone after.
  • Repro loop under a slow-thread-startup allocator: previously frequent hangs (~7/11) → 0 over 24 runs after both fixes. The atomic change alone was necessary but not sufficient — bug Results here are very hard to understand #2 needs the early-return fix too.

Two lines of behavioral change (std::atomic decls + finished = TRUE on early-return) plus #include <atomic>; no change to the benchmark's allocation behavior.

🤖 Generated with Claude Code

runthreads() spins on each slot's `finished` flag to detect worker completion,
but two bugs can leave a slot's `finished` false forever, hanging the loop:

1. Data race. `stopflag` and `thread_data::finished` are shared between the main
   thread (sets stopflag, reads finished) and the worker/respawn threads (read
   stopflag, write finished) as plain `volatile int`. `volatile` provides neither
   atomicity nor inter-thread ordering, so these are unsynchronized data races
   (undefined behavior) that ThreadSanitizer flags. Make both `std::atomic<int>`.

2. Missing completion on early-return. A worker that starts after main has set
   `stopflag = TRUE` hits `if (stopflag) return 0;` and returns WITHOUT setting
   `finished = TRUE`, leaving the slot's `finished` at its initial 0 forever. Mark
   the slot finished on that early-return.

Both are latent and timing-dependent: they surface intermittently when thread
startup/scheduling is slow enough to deliver a just-created worker after stopflag
is set (e.g. under an allocator whose per-thread setup delays a worker), while
faster allocators miss the window by luck. Verified with ThreadSanitizer (data
races eliminated) and by looping the repro (previously frequent hangs -> 0).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

@mjp41 mjp41 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants