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 <contact@withwoz.com>

* sync: fix ManyTimes ordering -- store count after f() completes, not before

The same ordering bug fixed in sync.Once (see #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 <contact@withwoz.com>

---------

Co-authored-by: WOZCODE <contact@withwoz.com>

Author: quaesitor-scientiam

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()
 }

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
+}

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()
 }

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
+}