I didn't intend Add to be used concurrently, by the way. One pool one
thread for the controller.
On Sunday, 5 May 2019 21:09:40 UTC+2, Louki Sumirniy wrote:
>
> If at 13 in the else clause, if I first test for a non-nil wg.ops and
> close it if it's open, I think that stops that channel leak.
>
> On Sunday, 5 May 2019 18:32:14 UTC+2, Marcin Romaszewicz wrote:
>>
>> 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.sumi...@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 golan...@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.
