sync: add example for Cond #20491
Comments
|
This might be a helpful example: https://github.com/golang/build/blob/master/livelog/livelog.go It's a bit too long for an inline doc, but maybe it can be condensed. |
|
I would expect that the reason the documentation and usage are pretty sparse in part because channels provide similar operations (and, unlike You're of course correct that |
|
This is what I'm thinking: https://play.golang.org/p/FkxA466uukw It uses both |
|
@awoodbeck The equivalent to that using channels is only a bit longer, and interoperates better with the So that's arguably still not a great use of |
|
This issue requested an example of using |
I disagree. The OP requested “a good example” in “ a somewhat realistic use”. A good example for an API should be a use-case for which it is actually the most appropriate tool, not just a technically-correct tool. It's relatively easy to come up with correct examples of how to use (This is in contrast to, say, |
|
@bcmills I understand your point. Thank you for the clarification. |
|
If we collectively fail to find an example where the use is appropriate,
documenting that is an equally valuable resolution. There are other
examples of deprecations that are kept due to 1.0 promise, perhaps
sync.Cond is just one of them?
That might be better than an example for a rarely used type, unless there
are indeed uses where it fits better.
Edit: I just noticed ongoing discussion in #21165 ; I understand the discussion so that there likely is appropriate use. If so, that documentation should be added. Thanks @bcmills
|
|
The only use case I come across sync.Cond for is when I want short lived periods of mutual exclusion over a named resource, a minimal example would be this. I think in many cases doing this with a goroutine / channel would be preferred for cancellation, but maybe this would serve as a fair enough example. |
|
@cstockton I'm looking at your example, but there's a detail that I don't understand: why is |
|
Actually, there's another detail I'm missing too: in At any rate, if the number of IDs is small and fixed it seems much clearer to use a long-lived channel per ID: If the number of IDs is large, a short-lived channel per ID would likely still lead to less overall contention on the mutex and more targeted wakeups, without significantly increasing steady-state memory usage: |
|
@bcmills No strong design considerations here, it was posted as an example ~6mo ago in response to a reddit question. I use that and the bound WaitGroup as somewhere to refer people to for how sync.Cond works. Since the purpose of this issue is to find a problem that serves as a good example for sync.Cond, providing solutions to the proposed problems using unrelated methods would probably add more value to your discussion to remove sync.Cond. While this primitive is a member of the standard library I see no harm in providing a clear example, so what do you feel is a better example of sync.Cond usage? That said both your suggestions add additional constraints, such as removing the guarantee that the value for the map value will be initialized only once. The second solution never deletes old ID's from the map, requiring IDS to be a fixed set that may fit in memory with an access frequency high enough to justify holding them forever. At that point you would just use a regular |
I don't think there is such a thing as a good example of
I think you've swapped “first” and “second”? At any rate, that's part of my point: It's true that my first suggestion retains keys that may no longer be interesting. In exchange, it avoids spurious wakeups: that is, it implements an O(N) solution for N calls to It's also true that my second suggestion allocates more than the In exchange for those changes in constraints, we get a few more nice properties: responsiveness to cancellation, usable zero-values, and reduction or elimination of spurious wakeups. The examples I gave are solutions in the design space that happen to fall at about the same line count as the So I don't think this example is a good one for |
An examples purpose is not to promote use, it's to illustrate how to use it. I do not understand what the harm is in having a clear example of how a sync.Cond works. If they understand the way a Cond is used in Go they may apply patterns they are familiar with from other languages for an easy to understand program.
Spurious wakeups are something to avoid- sure, but like all of your theory here they did not emerge as a significant details in the benchmarks I took the time to prove my position with. I also don't see the use of 10 lines of software written in under a minute in a pattern I have long understood, can prove is correct while also getting good general performance... somehow limiting future exploration of alternative solutions.
You are pushing an agenda to remove sync.Cond under the premise:
Only thing you proved here is a demand for documentation. It's unfortunate you won't apply the expertise you've demonstrated to contribute a meaningful example you feel would best highlight correct usage. Showing someone how to do something properly, doesn't mean you are telling them when to do it. |
I already addressed that above:
A zero
I think you have mistaken the cause and effect of my reasoning. Because I cannot find a |
While you are free to call the usage of the New func optional, the simple fact is the only documented and consistent way every single usage of sync.Pool I have ever seen initializes this value. So I find requiring initialization not being a sufficient reason for removing a sync prim from the std library.
This reads to me in the context of this post that he's talking about getting the GC to the point it's sophisticated enough to no longer need sync.Pool. But that is just my interpretation- I'll defend my positions with the points I used to create them instead of inserting my views half way through the sentence of someone else. I'll admit I didn't think I would go 3 comments into an issue for my favorite language debating the merit of improving documentation of all things. But it's clear you're not budging here so I'll agree to disagree, thanks. |
|
@cstockton Here's an example of https://github.com/golang/exp/blob/master/shiny/driver/internal/event/event.go Disclaimer: I don't like |
|
I don't think anybody likes
On the other hand Anyhow I think the deque is not a bad example if we stress the ability to support a varying amount of data. |
|
The It has a couple of issues unrelated to It does have one (minor) issue related to Given that the entire Shiny framework is not plumbed for contexts, Finally, I'll note that the alternative using channels (https://golang.org/cl/94138) is not particularly worse, and has the advantages of both avoiding spurious wakeups and allowing for cancellation support more easily down the road. |
I do want to note that my alternative for the I would argue that the real cases where |
|
It is a lot more pleasant to deal with the events in Go if each has a distinct channel instead of being stored in one deque. The latter is more familiar to event driven UI development, but the former seems more appropriate to what channels did in earlier languages that had them. |
|
I now have lots of examples of what to do instead of To revisit @ianlancetaylor's specific points, based on what I learned preparing for that talk:
Both of those are pretty solidly addressed by using a 1-buffered channel. The items stored in the channel can refer to arbitrarily large backing structures, and readers can use a The channel does add a layer of indirection, but then so does |
|
How about just a simple example guys? This one from SO taught me infinitely more about how sync.Cond works than the Go docs: https://stackoverflow.com/questions/36857167/how-to-correctly-use-sync-cond# The debate about proper, real world use (if that even exists) can live in another issue, like this one: #21165 We just need something that walks people through the basic flow of using the methods. That's the foundation people can use to determine whether it makes sense to use it. |
|
Change https://go.dev/cl/412237 mentions this issue: |
Cond is difficult to use correctly (I was just bitten by it in a production app that I inherited). While several proposals have come up to improve or remove sync.Cond, no action has so far been taken. Update the documentation to discourage use of sync.Cond, and point people in the direction of preferred alternatives. I believe this will help encourage behavior we want (less use of sync.Cond and more use of channels), while also paving the way for, potentially, removing Cond in a future version of the language. Thanks very much to Bryan Mills and Sean Liao for discussion and recommendations. Updates #20491. Updates #21165. Change-Id: Ib4d0631c79d4c4d0a30027255cd43bc47cddebd3 Reviewed-on: https://go-review.googlesource.com/c/go/+/412237 Run-TryBot: Ian Lance Taylor <[email protected]> Auto-Submit: Ian Lance Taylor <[email protected]> Reviewed-by: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
gopherbot
added
the
compiler/runtime
|
Change https://go.dev/cl/521596 mentions this issue: |
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 <[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]>
|
Change https://go.dev/cl/521598 mentions this issue: |
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]>
…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
seankhliao
added
the
NeedsInvestigation
The sync.Cond should have an example demonstrating its use in a somewhat realistic use.
What would be a good example? It's hardly used at all in standard library. What are typical uses?
Continuation from #20471.
The text was updated successfully, but these errors were encountered: