I've done quite a bit of MP programming over 20+ years now, and skimming
your code, I see a number of issues. There are probably a lot more that I
don't. In the latest go playground link
Line14: if "if wg.started {" has a race condition, both on accessing the
variable, and logically, in that two goroutines can come into the function
and both go down the wg.started = false path. You can't do this without a
synchronization primitive.
Line 17: (wg.ops = make(chan int)) Because of line 14, you could create
more than one channel here, and different go routines will read a different
wg.ops, so your WaitGroup won't work. You also have a data race in
assigning wg.ops because wg.ops isn't an atomic type.
Line 41: op, ok := <-wg.ops . You could be waiting forever because of
leaked channel in line 17
It seems like your code works for a simple test, but if you really hammer
on this thing, it's going to fail.
-- Marcin
On Sun, May 5, 2019 at 8:33 AM Louki Sumirniy <
louki.sumirniy.stal...@gmail.com> wrote:
> just had to drop an update, I further improved it, now when it stops it
> resets itself and you can use Add again, I removed the unnecessary
> positive/negative checks and condensed the Add and Done function into an
> Add that can take a negative (tried to think of a better word but Modify
> and Change... just didn't quite fit)
>
> https://play.golang.org/p/cO3sV1w9-Re
>
> One could add back separate Add and Done functions, even conveniences that
> just inc 1 dec 1 for each but I don't see the point in that. It just simply
> allows you to make Wait block while the count is above zero, which is all
> you need to prevent goroutine leaks.
>
> On Sunday, 5 May 2019 17:18:52 UTC+2, Louki Sumirniy wrote:
>>
>> I figured out that the API of my design had a flaw separating starting
>> the goroutine and adding a new item, so as you can see in this code, I have
>> merged them and notice that there is basically no extraneous 'helper'
>> functions also:
>>
>> https://play.golang.org/p/hR1sTOAwkOm
>>
>> The flaw I made relates to API abuse - the contract of the library is
>> quite simply to concurrently keep track of the adjacent 'go' calls starting
>> goroutines, which doesn't apply until there is an increment. So you should
>> not be able to abuse this one the way you did the last.
>>
>> I'm pretty sure at somewhere between 50 and 100 worker routines the lack
>> of complex extra code makes this a better implementation. Probably the
>> original is fine for smaller worker pools.
>>
>> On Sunday, 5 May 2019 13:01:58 UTC+2, Jan Mercl wrote:
>>>
>>> On Sun, May 5, 2019 at 12:45 PM Louki Sumirniy
>>> <louki.sumi...@gmail.com> wrote:
>>> >
>>> > https://play.golang.org/p/5KwJQcTsUPg
>>> >
>>> > I fixed it.
>>>
>>> Not really. You've introduced a data race.
>>>
>>> jnml@4670:~/src/tmp> cat main.go
>>> package main
>>>
>>> type WaitGroup struct {
>>> workers int
>>> ops chan int
>>> }
>>>
>>> func New() *WaitGroup {
>>> wg := new(WaitGroup)
>>> return wg
>>> }
>>>
>>> func startWait(wg *WaitGroup) {
>>> wg.ops = make(chan int)
>>>
>>> go func() {
>>> done := false
>>> for !done {
>>> select {
>>> case op := <-wg.ops:
>>> wg.workers += op
>>> if wg.workers < 1 {
>>> done = true
>>> close(wg.ops)
>>> }
>>> }
>>> }
>>> }()
>>> }
>>>
>>> // Add adds a non-negative number
>>> func (wg *WaitGroup) Add(delta int) {
>>> if delta < 0 {
>>> return
>>> }
>>> if wg.ops == nil {
>>> startWait(wg)
>>> }
>>> wg.ops <- delta
>>> }
>>>
>>> // Done subtracts a non-negative value from the workers count
>>> func (wg *WaitGroup) Done(delta int) {
>>> if delta < 0 {
>>> return
>>> }
>>> wg.ops <- -delta
>>> }
>>>
>>> // Wait blocks until the waitgroup decrements to zero
>>> func (wg *WaitGroup) Wait() {
>>> for {
>>> if wg.workers < 1 {
>>> break
>>> }
>>> op, ok := <-wg.ops
>>> if !ok {
>>> break
>>> } else {
>>> wg.ops <- op
>>> }
>>> }
>>> }
>>>
>>> var wg = New()
>>>
>>> func main() {
>>> for i := 0; i < 2; i++ {
>>> wg.Add(1)
>>> go f()
>>> }
>>> wg.Wait()
>>> }
>>>
>>> func f() {
>>> defer wg.Done(1)
>>>
>>> for i := 0; i < 2; i++ {
>>> wg.Add(1)
>>> go g()
>>> }
>>> }
>>>
>>> func g() { wg.Done(1) }
>>> jnml@4670:~/src/tmp> go run -race main.go
>>> ==================
>>> WARNING: DATA RACE
>>> Read at 0x00c00008a000 by main goroutine:
>>> main.(*WaitGroup).Wait()
>>> /home/jnml/src/tmp/main.go:53 +0x6f
>>> main.main()
>>> /home/jnml/src/tmp/main.go:72 +0x104
>>>
>>> Previous write at 0x00c00008a000 by goroutine 5:
>>> main.startWait.func1()
>>> /home/jnml/src/tmp/main.go:21 +0xbb
>>>
>>> Goroutine 5 (running) created at:
>>> main.startWait()
>>> /home/jnml/src/tmp/main.go:16 +0x9e
>>> main.main()
>>> /home/jnml/src/tmp/main.go:37 +0xdf
>>> ==================
>>> Found 1 data race(s)
>>> exit status 66
>>> jnml@4670:~/src/tmp>
>>>
>> --
> You received this message because you are subscribed to the Google Groups
> "golang-nuts" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-nuts+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
