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
> <javascript:>> 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 <javascript:>.
>> 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.
