From d6baaceefab0a307b1d7ae8f8bf92cfa5e03a854 Mon Sep 17 00:00:00 2001
From: Terin Stock <terinjokes@gmail.com>
Date: Thu, 2 Mar 2023 00:05:26 +0100
Subject: fix: manager started race

It's possible for there to be a data race where `Add` checks if the
manager has started, then proceeds to append to the runnables slice, at
the same time that `Start` is progressing towards starting the manager.
This results in a data race with the runnables slice.

This changeset modifies the manager to use a more traditional locking approach.
---
 manager/manager.go | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/manager/manager.go b/manager/manager.go
index 1a79214..704bced 100644
--- a/manager/manager.go
+++ b/manager/manager.go
@@ -5,13 +5,15 @@ package manager
 
 import (
 	"context"
-	"sync/atomic"
+	"errors"
+	"sync"
 
 	"golang.org/x/sync/errgroup"
 )
 
 type Manager struct {
-	started     uint32
+	mu          sync.Mutex
+	started     bool
 	runnables   []Runnable
 	internalCtx context.Context
 	group       *errgroup.Group
@@ -24,7 +26,10 @@ func New() *Manager {
 }
 
 func (m *Manager) Add(r Runnable) error {
-	if atomic.LoadUint32(&m.started) == 1 {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+
+	if m.started {
 		m.group.Go(func() error {
 			return r.Start(m.internalCtx)
 		})
@@ -36,11 +41,16 @@ func (m *Manager) Add(r Runnable) error {
 }
 
 func (m *Manager) Start(ctx context.Context) error {
+	m.mu.Lock()
+	if m.started {
+		m.mu.Unlock()
+		return errors.New("manager already started")
+	}
+
 	g, internalCtx := errgroup.WithContext(ctx)
 	m.group = g
 	m.internalCtx = internalCtx
 
-	atomic.SwapUint32(&m.started, 1)
 	for _, r := range m.runnables {
 		r := r
 		g.Go(func() error {
@@ -48,6 +58,9 @@ func (m *Manager) Start(ctx context.Context) error {
 		})
 	}
 	m.runnables = nil
+	m.started = true
+
+	m.mu.Unlock()
 
 	return g.Wait()
 }
-- 
cgit 1.4.1