From ef0b159c5cd83290cb3a6d6d5105d4872e14c1b4 Mon Sep 17 00:00:00 2001 From: Alexandre Perrin Date: Thu, 19 May 2022 17:39:08 +0200 Subject: [PATCH] workerpool: WaitGroup synchronization fix 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 --- workerpool.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/workerpool.go b/workerpool.go index d640cb1..71d262a 100644 --- a/workerpool.go +++ b/workerpool.go @@ -94,9 +94,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,