context: fix synchronization in ExampleAfterFunc_cond

Condition variables are subtle and error-prone, and this example
demonstrates exactly the sorts of problems that they introduce.
Unfortunately, we're stuck with them for the foreseeable future.

As previously implemented, this example was racy: since the callback
passed to context.AfterFunc did not lock the mutex before calling
Broadcast, it was possible for the Broadcast to occur before the
goroutine was parked in the call to Wait, causing in a missed wakeup
resulting in deadlock.

The example also had a more insidious problem: it was not safe for
multiple goroutines to call waitOnCond concurrently, but the whole
point of using a sync.Cond is generally to synchronize concurrent
goroutines. waitOnCond must use Broadcast to ensure that it wakes up
the target goroutine, but the use of Broadcast in this way would
produce spurious wakeups for all of the other goroutines waiting on
the same condition variable. Since waitOnCond did not recheck the
condition in a loop, those spurious wakeups would cause waitOnCond
to spuriously return even if its own ctx was not yet done.

Fixing the aforementioned bugs exposes a final problem, inherent to
the use of condition variables in this way. This one is a performance
problem: for N concurrent calls to waitOnCond, the resulting CPU cost
is at least O(N²). This problem cannot be addressed without either
reintroducing one of the above bugs or abandoning sync.Cond in the
example entirely. Given that this example was already published in Go
1.21, I worry that Go users may think that it is appropriate to use a
sync.Cond in conjunction with context.AfterFunc, so I have chosen to
retain the Cond-based example and document its pitfalls instead of
removing or replacing it entirely.

I described this class of bugs and performance issues — and suggested
some channel-based alternatives — in my GopherCon 2018 talk,
“Rethinking Classical Concurrency Patterns”. The section on condition
variables starts on slide 37. (https://youtu.be/5zXAHh5tJqQ?t=679)

Fixes #62180.
For #20491.

Change-Id: If987cd9d112997c56171a7ef4fccadb360bb79bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/521596
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>

diff --git a/src/context/example_test.go b/src/context/example_test.go
index 38549a12de898e19f39448bf01f37209acdb74c9..03333b5ccae9599eb2d0cd179312d32830081e9b 100644 (file)
--- a/src/context/example_test.go
+++ b/src/context/example_test.go
@@ -125,25 +125,64 @@ func ExampleWithValue() {
 // This example uses AfterFunc to define a function which waits on a sync.Cond,
 // stopping the wait when a context is canceled.
 func ExampleAfterFunc_cond() {
-       waitOnCond := func(ctx context.Context, cond *sync.Cond) error {
-               stopf := context.AfterFunc(ctx, cond.Broadcast)
+       waitOnCond := func(ctx context.Context, cond *sync.Cond, conditionMet func() bool) error {
+               stopf := context.AfterFunc(ctx, func() {
+                       // We need to acquire cond.L here to be sure that the Broadcast
+                       // below won't occur before the call to Wait, which would result
+                       // in a missed signal (and deadlock).
+                       cond.L.Lock()
+                       defer cond.L.Unlock()
+
+                       // If multiple goroutines are waiting on cond simultaneously,
+                       // we need to make sure we wake up exactly this one.
+                       // That means that we need to Broadcast to all of the goroutines,
+                       // which will wake them all up.
+                       //
+                       // If there are N concurrent calls to waitOnCond, each of the goroutines
+                       // will spuriously wake up O(N) other goroutines that aren't ready yet,
+                       // so this will cause the overall CPU cost to be O(N²).
+                       cond.Broadcast()
+               })
                defer stopf()
-               cond.Wait()
-               return ctx.Err()
+
+               // Since the wakeups are using Broadcast instead of Signal, this call to
+               // Wait may unblock due to some other goroutine's context becoming done,
+               // so to be sure that ctx is actually done we need to check it in a loop.
+               for !conditionMet() {
+                       cond.Wait()
+                       if ctx.Err() != nil {
+                               return ctx.Err()
+                       }
+               }
+
+               return nil
        }
 
-       ctx, cancel := context.WithTimeout(context.Background(), 1*time.Millisecond)
-       defer cancel()
+       cond := sync.NewCond(new(sync.Mutex))
+
+       var wg sync.WaitGroup
+       for i := 0; i < 4; i++ {
+               wg.Add(1)
+               go func() {
+                       defer wg.Done()
 
-       var mu sync.Mutex
-       cond := sync.NewCond(&mu)
+                       ctx, cancel := context.WithTimeout(context.Background(), 1*time.Millisecond)
+                       defer cancel()
 
-       mu.Lock()
-       err := waitOnCond(ctx, cond)
-       fmt.Println(err)
+                       cond.L.Lock()
+                       defer cond.L.Unlock()
+
+                       err := waitOnCond(ctx, cond, func() bool { return false })
+                       fmt.Println(err)
+               }()
+       }
+       wg.Wait()
 
        // Output:
        // context deadline exceeded
+       // context deadline exceeded
+       // context deadline exceeded
+       // context deadline exceeded
 }
 
 // This example uses AfterFunc to define a function which reads from a net.Conn,