From df24ab237ef04ddaf535d9b7253cfa8773771816 Mon Sep 17 00:00:00 2001 From: Richard Wheeler Date: Mon, 22 Jun 2026 07:53:11 -0400 Subject: [PATCH] sync: fix ordering in Once and ManyTimes -- store count after f() completes (#27490) * sync: fix Once ordering -- store done flag after f() completes, not before Previously do_slow/do_slow_with_param set count=1 before calling f(), so a concurrent goroutine could observe count==1 on the fast path and return from do() while f() was still executing. This broke the fundamental Once guarantee that when do() returns, f() side effects are visible. Fix: move the stdatomic.store_u64 to after f() returns, matching the ordering used by Go sync.Once. Concurrent callers that arrive while f() is running now correctly block on the mutex until f() completes. Adds a test that would fail with the old ordering: the second do_with_param call must not return until slow_init has set its value. Fixes #27456. Co-Authored-By: WOZCODE * sync: fix ManyTimes ordering -- store count after f() completes, not before The same ordering bug fixed in sync.Once (see vlang/v#27456): `do_slow` was incrementing `count` before calling `f()`, so a concurrent goroutine could observe `count >= times` on the atomic fast path and return from `do` while `f()` was still executing. Move `stdatomic.store_u64` to after `f()` so that `count` is only incremented once the work is complete. Adds a concurrency ordering test analogous to `test_once_with_param_ordering` in `once_with_param_test.v`. Co-Authored-By: WOZCODE --------- Co-authored-by: WOZCODE --- vlib/sync/many_times.v | 2 +- vlib/sync/many_times_test.v | 43 +++++++++++++++++++++++++++++ vlib/sync/once.v | 13 ++++++--- vlib/sync/once_with_param_test.v | 47 ++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 5 deletions(-) diff --git a/vlib/sync/many_times.v b/vlib/sync/many_times.v index a82d63cfd..9efd5de1c 100644 --- a/vlib/sync/many_times.v +++ b/vlib/sync/many_times.v @@ -29,8 +29,8 @@ pub fn (mut m ManyTimes) do(f fn ()) { fn (mut m ManyTimes) do_slow(f fn ()) { m.m.lock() if m.count < m.times { - stdatomic.store_u64(&m.count, m.count + 1) f() + stdatomic.store_u64(&m.count, m.count + 1) } m.m.unlock() } diff --git a/vlib/sync/many_times_test.v b/vlib/sync/many_times_test.v index 05c39d256..4dd892fcf 100644 --- a/vlib/sync/many_times_test.v +++ b/vlib/sync/many_times_test.v @@ -1,5 +1,6 @@ // vtest build: !windows && (amd64 || arm64) import sync +import time struct Counter { pub mut: @@ -48,3 +49,45 @@ fn test_many_times_fifth() { } assert co.i == 25 } + +// Ordering test: do must not return until f() has completed. +// The bug was that count was stored *before* f() ran, so a concurrent +// caller could observe count >= times and return on the fast path while +// f() was still executing. + +struct ManyTimesState { +pub mut: + value int + ready chan bool +} + +fn run_mt_ordering(mut m sync.ManyTimes, s &ManyTimesState, c chan bool) { + m.do(fn [s] () { + mut ms := unsafe { &ManyTimesState(s) } + ms.ready <- true + time.sleep(50 * time.millisecond) + ms.value = 99 + }) + c <- true +} + +fn test_many_times_ordering() { + s := &ManyTimesState{ + ready: chan bool{cap: 1} + } + mut m := sync.new_many_times(1) + c := chan bool{} + + spawn run_mt_ordering(mut m, s, c) + + // Wait until the goroutine is inside f(). With the old buggy code, + // count is incremented before f() runs, so the second do() call sees + // count >= times on the fast path and returns immediately — before + // f() has finished. With the fix, count is still 0 here, so the + // second do() blocks on the mutex until f() completes. + _ := <-s.ready + m.do(fn () {}) + + assert s.value == 99, 'do must not return before f() completes' + _ := <-c +} diff --git a/vlib/sync/once.v b/vlib/sync/once.v index 434931d53..785bb2560 100644 --- a/vlib/sync/once.v +++ b/vlib/sync/once.v @@ -16,7 +16,9 @@ pub fn new_once() &Once { return once } -// do executes the function `f()` only once. +// do executes the function `f()` only once, regardless of how many goroutines +// call it concurrently. When do returns, f() has completed. +// Note: if f() calls do on the same Once, it will deadlock. pub fn (mut o Once) do(f fn ()) { if stdatomic.load_u64(&o.count) < 1 { o.do_slow(f) @@ -26,8 +28,8 @@ pub fn (mut o Once) do(f fn ()) { fn (mut o Once) do_slow(f fn ()) { o.m.lock() if o.count < 1 { - stdatomic.store_u64(&o.count, 1) f() + stdatomic.store_u64(&o.count, 1) } o.m.unlock() } @@ -49,7 +51,10 @@ fn (mut o Once) do_slow(f fn ()) { // }, o) // ``` -// do_with_param executes the function `f()` with parameter `param` only once. +// do_with_param executes the function `f(param)` only once, regardless of how +// many goroutines call it concurrently. When do_with_param returns, f() has +// completed. +// Note: if f() calls do_with_param on the same Once, it will deadlock. pub fn (mut o Once) do_with_param(f fn (voidptr), param voidptr) { if stdatomic.load_u64(&o.count) < 1 { o.do_slow_with_param(f, param) @@ -59,8 +64,8 @@ pub fn (mut o Once) do_with_param(f fn (voidptr), param voidptr) { fn (mut o Once) do_slow_with_param(f fn (p voidptr), param voidptr) { o.m.lock() if o.count < 1 { - stdatomic.store_u64(&o.count, 1) f(param) + stdatomic.store_u64(&o.count, 1) } o.m.unlock() } diff --git a/vlib/sync/once_with_param_test.v b/vlib/sync/once_with_param_test.v index c2acdfb6b..ef8f933eb 100644 --- a/vlib/sync/once_with_param_test.v +++ b/vlib/sync/once_with_param_test.v @@ -1,4 +1,5 @@ import sync +import time // Note: this is the same test as `vlib/sync/once_test.v`, but // it uses an explicit passing of the voidptr parameter in @@ -36,3 +37,49 @@ fn test_once() { } assert o.i == 5 } + +// Ordering test: do_with_param must not return until f() has completed. +// The bug was that count was set to 1 *before* f() ran, so a concurrent +// caller could observe count==1 and return on the fast path while f() was +// still executing (or hadn't started). + +struct OrderingState { +pub mut: + value int + ready chan bool +} + +fn slow_init(p voidptr) { + mut s := unsafe { &OrderingState(p) } + // Signal that we are inside f() — count has been stored (buggy) or not (fixed). + s.ready <- true + // Sleep long enough for the concurrent do_with_param call to observe the state. + time.sleep(50 * time.millisecond) + s.value = 99 +} + +fn run_ordering(mut once sync.Once, s &OrderingState, c chan bool) { + once.do_with_param(slow_init, s) + c <- true +} + +fn test_once_with_param_ordering() { + mut s := &OrderingState{ + ready: chan bool{cap: 1} + } + mut once := sync.new_once() + c := chan bool{} + + spawn run_ordering(mut once, s, c) + + // Wait until the goroutine is inside f() before letting the second + // caller proceed. With the old buggy code count==1 is already visible + // here, so the second do_with_param returns immediately — before + // s.value is set. With the fix, count is still 0, so the second + // caller blocks on the mutex and waits for f() to complete. + _ := <-s.ready + once.do_with_param(fn (p voidptr) {}, unsafe { nil }) + + assert s.value == 99, 'do_with_param must not return before f() completes' + _ := <-c +} -- 2.39.5