fix: Fix deadlock raised in #11 #13
Conversation
| @@ -40,8 +40,10 @@ type Group struct { | |||
|
|
|||
| wg sync.WaitGroup | |||
|
|
|||
| errOnce sync.Once | |||
There was a problem hiding this comment.
Since now we will be reading err continuously then we needed an RWMutex already.
We could keep the sync.Once but the same can be achieved with RWMutex that is already needed, so removed the sync.Once. Let me know if you think it's cleaner to keep both sync.Once and sync.RWMutex.
| @@ -154,13 +156,28 @@ func (g *Group) Go(f func() error) { | |||
| return | |||
| } | |||
|
|
|||
| g.qCh <- f | |||
| for { | |||
There was a problem hiding this comment.
In this for loop we simply do the following:
- Get a read lock on err
- If err is NOT nil we return as there is no need to send function to buffer
- If err is nil we try to send to channel, if we can't send to channel then we break the select and go back to checking if err is not nil
There was a problem hiding this comment.
The only concern with this is it could become a busy loop. I wonder if sync.Cond can do something better here
| @@ -204,16 +221,30 @@ func (g *Group) startG() { | |||
| return | |||
| } | |||
|
|
|||
| if err := f(); err != nil { | |||
There was a problem hiding this comment.
This looks nicer but as explained above we are already needing the RWMutex so this can be achieved by using the RWMutex instead of also introducing a new field sync.Once.
So let me know if you think its better to stay using sync.Once in addition to the RWMutex just for readability.
|
As stated here this can be tried out using go mod edit -replace github.com/neilotoole/errgroup=github.com/alpacahq/errgroup@master |
| err error | ||
|
|
||
| // errMu protects err. | ||
| errMu sync.RWMutex |
This fixes #11 .
Root cause:
startGfunction.gCountreaches 0 while all other functions are already in the buffer then a deadlock will be reached since there is no chance a new worker will be created and the current number of workers is 0.Fix: (Note: there are different ways of achieving this)