context: misuse of sync.Cond in ExampleAfterFunc_cond
#62180
Assignees
Labels
Documentation
Issues describing a change to documentation.
FrozenDueToAge
NeedsFix
The path to resolution is known, but the work has not been done.
Testing
An issue that has been verified to require only test changes, not just a test failure.
Milestone
Comments
bcmills
added
the
NeedsInvestigation
|
Found new dashboard test flakes for: 2023-08-07 22:48 openbsd-arm64-jsing go@b9153f6e context (log) |
|
@gopherbot, please backport to Go 1.21. This example in the |
bcmills
added
Testing
|
Backport issue(s) opened: #62189 (for 1.21). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
gopherbot
removed
the
NeedsInvestigation
bcmills
changed the title
ExampleAfterFunc_condExampleAfterFunc_cond
bcmills
changed the title
ExampleAfterFunc_condsync.Cond in ExampleAfterFunc_cond
|
Change https://go.dev/cl/521596 mentions this issue: |
|
I will send a fix. Further evidence that |
|
Change https://go.dev/cl/521598 mentions this issue: |
cellularmitosis
pushed a commit
to cellularmitosis/go
that referenced
this issue
Aug 24, 2023
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 golang#62180. For golang#20491. Change-Id: If987cd9d112997c56171a7ef4fccadb360bb79bc Reviewed-on: https://go-review.googlesource.com/c/go/+/521596 Reviewed-by: Cuong Manh Le <[email protected]> Auto-Submit: Bryan Mills <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Bryan Mills <[email protected]>
gopherbot
pushed a commit
that referenced
this issue
Aug 24, 2023
…unc_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 #62189. Updates #62180. For #20491. Change-Id: If987cd9d112997c56171a7ef4fccadb360bb79bc Reviewed-on: https://go-review.googlesource.com/c/go/+/521596 Reviewed-by: Cuong Manh Le <[email protected]> Auto-Submit: Bryan Mills <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Bryan Mills <[email protected]> (cherry picked from commit 1081f8c) Reviewed-on: https://go-review.googlesource.com/c/go/+/521598
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
(Pulled out from #62165 (comment).)
https://build.golang.org/log/6040fd74316c6fe15adcca97b7b68a471db61575:
This test / example was added in https://go.dev/cl/482695 (attn @dneil @Sajmani) for #57928; it's new as of Go 1.21.
Given that this example involves a
sync.Condand acontext.WithTimeout, I'm about 98% confident that the bug is in the example somewhere. 😅The text was updated successfully, but these errors were encountered: