Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

context: misuse of sync.Cond in ExampleAfterFunc_cond [1.21 backport] #62189

Closed
gopherbot opened this issue Aug 21, 2023 · 3 comments
Closed
Labels
CherryPickApproved Used during the release process for point releases Documentation Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@gopherbot
Copy link

@bcmills requested issue #62180 to be considered for backport to the next 1.21 minor release.

@gopherbot, please backport to Go 1.21. This example in the context documentation for an API added in Go 1.21 demonstrates incorrect use of a sync.Cond.

@gopherbot gopherbot added CherryPickCandidate Used during the release process for point releases Testing An issue that has been verified to require only test changes, not just a test failure. labels Aug 21, 2023
@gopherbot gopherbot added this to the Go1.21.1 milestone Aug 21, 2023
@bcmills bcmills changed the title context: apparent deadlock in ExampleAfterFunc_cond [1.21 backport] context: misuse of sync.Cond in ExampleAfterFunc_cond [1.21 backport] Aug 21, 2023
@gopherbot
Copy link
Author

Change https://go.dev/cl/521598 mentions this issue: [release-branch.go1.21] context: fix synchronization in ExampleAfterFunc_cond

@cagedmantis cagedmantis added the CherryPickApproved Used during the release process for point releases label Aug 23, 2023
@cagedmantis
Copy link
Contributor

Approved as it is a documentation change that is necessary.

@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Aug 23, 2023
@gopherbot
Copy link
Author

Closed by merging 2977709 to release-branch.go1.21.

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 <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>
(cherry picked from commit 1081f8c)
Reviewed-on: https://go-review.googlesource.com/c/go/+/521598
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CherryPickApproved Used during the release process for point releases Documentation Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

3 participants