Codecov Report

Attention: Patch coverage is 91.04478% with 6 lines in your changes missing coverage. Please review.

Project coverage is 95.27%. Comparing base (1ad365a) to head (0ccf029).
Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
+ Coverage   94.62%   95.27%   +0.65%     
==========================================
  Files          11       11              
  Lines         725      720       -5     
==========================================
  Hits          686      686              
+ Misses         35       31       -4     
+ Partials        4        3       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hey @alitto !

I prefer your solution over mine for its simplicity. It reduces the chance of a data race significantly, but doesn't eliminate it completely because WaitGroup.Add and WaitGroup.Wait still stay unsynchronized.

Please take a look at a slightly modified test from my PR that proves data race is still there (better to run this one standalone — go test -race -v -timeout 15s -count=1 -test.run '^TestSubmitClose$'):

func TestSubmitClose(t *testing.T) {
	pool := NewPool(0)

	wg := sync.WaitGroup{}

	wg.Add(1)
	go func() {
		defer wg.Done()

		time.Sleep(500 * time.Microsecond)
		pool.StopAndWait()
	}()

	for i := 0; i < 100000; i++ {
		wg.Add(1)
		go func() {
			defer wg.Done()

			pool.Submit(func() {
				time.Sleep(10 * time.Millisecond)
			})
		}()
	}

	wg.Wait()
}

IIUC, the unlock operation needs to be done at the end of trySubmit?

@liznear that seems legit to me.

Thanks @korotin for providing that test 🙌, i added it to pool_test.go to ensure there's no data race around workersWaitGroup.
@liznear unlocking at the end of trySubmit would do the trick but after some rearrangements of the code in that method I was able to make sure all calls to p.workersWaitGroup.Add(1) are made while holding the lock, so it should be equivalent to what you are proposing. The key difference is the lock is not held while launching the goroutine, but that's not really necessary to avoid a data race here so better to keep it outside.
I ended up refactoring the trySubmit method to reduce the number of branches and move all if statements to the top. Also, now all calls to increment/decrement workerCount and workersWaitGroup are grouped together and always performed while holding the lock. It looks cleaner and it should prevent data races. While doing this I noticed workerCount was not being decremented when the context was cancelled, so this PR fixes that too.
I left calls to workersWaitGroup.Wait() not synchronized because it's not necessary given the swapping of the closed flag is synchronized. This ensures there won't be any calls to workersWaitGroup.Add(1) once closed is set to true, because this flag is checked before deciding to launch a new worker in a synchronized manner.
Thoughts?

@alitto seems like a nice and clean solution! 😎