Skip to content

Commit

Permalink
workerpool: WaitGroup synchronization fix
Browse files Browse the repository at this point in the history
Before this patch, Submit() would increment the WaitGroup without
holding the WorkerPool lock. This could potentially lead to a panic when
Submit() and Close() were executed concurrently:

1. The Submit() goroutine g0 acquire the lock, successfully check that
   the WorkerPool is neither closed nor draining, and release the lock.
2. The Close() goroutine g1 acquire the lock, update the WorkerPool
   state to closed, wait on the WaitGroup, and close the tasks channel
   (wrongly) assuming that all tasks have been submitted.
3. g0 increment the WaitGroup, and attempt to write to the (now closed)
   tasks channel leading to a panic.

The same reasoning can be applied with Submit() and Drain() leading to a
data race on the results slice.

Signed-off-by: Alexandre Perrin <[email protected]>
  • Loading branch information
kaworu committed May 20, 2022
1 parent 4cf96b5 commit 90fa389
Showing 1 changed file with 1 addition and 2 deletions.
3 changes: 1 addition & 2 deletions workerpool.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,8 @@ func (wp *WorkerPool) Submit(id string, f func(ctx context.Context) error) error
wp.mu.Unlock()
return ErrDraining
}

wp.mu.Unlock()
wp.wg.Add(1)
wp.mu.Unlock()
wp.tasks <- &task{
id: id,
run: f,
Expand Down

0 comments on commit 90fa389

Please sign in to comment.