sync: fix spurious wakeup from WaitGroup.Wait

rui314 · dvyukov · commit e9347c781be6 · 2014-04-10T18:44:44.000+04:00
There is a race condition that causes spurious wakeup from Wait in the following case: G1: decrement wg.counter, observe the counter is now 0 (should unblock goroutines queued *at this moment*) G2: increment wg.counter G2: call Wait() to add itself to the wait queue G1: acquire wg.m, unblock all waiting goroutines In the last step G2 is spuriously woken up by G1. Fixes #7734. LGTM=rsc, dvyukov R=dvyukov, 0xjnml, rsc CC=golang-codereviews https://golang.org/cl/85580043
diff --git a/src/pkg/sync/waitgroup.go b/src/pkg/sync/waitgroup.go
@@ -67,11 +67,13 @@ func (wg *WaitGroup) Add(delta int) {
 		return
 	}
 	wg.m.Lock()
-	for i := int32(0); i < wg.waiters; i++ {
-		runtime_Semrelease(wg.sema)
+	if atomic.LoadInt32(&wg.counter) == 0 {
+		for i := int32(0); i < wg.waiters; i++ {
+			runtime_Semrelease(wg.sema)
+		}
+		wg.waiters = 0
+		wg.sema = nil
 	}
-	wg.waiters = 0
-	wg.sema = nil
 	wg.m.Unlock()
 }
 
diff --git a/src/pkg/sync/waitgroup_test.go b/src/pkg/sync/waitgroup_test.go
@@ -6,6 +6,7 @@ package sync_test
 
 import (
 	. "sync"
+	"sync/atomic"
 	"testing"
 )
 
@@ -59,6 +60,31 @@ func TestWaitGroupMisuse(t *testing.T) {
 	t.Fatal("Should panic")
 }
 
+func TestWaitGroupRace(t *testing.T) {
+	// Run this test for about 1ms.
+	for i := 0; i < 1000; i++ {
+		wg := &WaitGroup{}
+		n := new(int32)
+		// spawn goroutine 1
+		wg.Add(1)
+		go func() {
+			atomic.AddInt32(n, 1)
+			wg.Done()
+		}()
+		// spawn goroutine 2
+		wg.Add(1)
+		go func() {
+			atomic.AddInt32(n, 1)
+			wg.Done()
+		}()
+		// Wait for goroutine 1 and 2
+		wg.Wait()
+		if atomic.LoadInt32(n) != 2 {
+			t.Fatal("Spurious wakeup from Wait")
+		}
+	}
+}
+
 func BenchmarkWaitGroupUncontended(b *testing.B) {
 	type PaddedWaitGroup struct {
 		WaitGroup

Original file line number	Diff line number	Diff line change
`@@ -67,11 +67,13 @@ func (wg *WaitGroup) Add(delta int) {`
`67`	`67`	`return`
`68`	`68`	`}`
`69`	`69`	`wg.m.Lock()`
`70`		`- for i := int32(0); i < wg.waiters; i++ {`
`71`		`- runtime_Semrelease(wg.sema)`
	`70`	`+ if atomic.LoadInt32(&wg.counter) == 0 {`
	`71`	`+ for i := int32(0); i < wg.waiters; i++ {`
	`72`	`+ runtime_Semrelease(wg.sema)`
	`73`	`+ }`
	`74`	`+ wg.waiters = 0`
	`75`	`+ wg.sema = nil`
`72`	`76`	`}`
`73`		`- wg.waiters = 0`
`74`		`- wg.sema = nil`
`75`	`77`	`wg.m.Unlock()`
`76`	`78`	`}`
`77`	`79`