From c853715be13aa0eb985a2ebf2af7f717da691389 Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Wed, 28 Jun 2023 12:09:42 +0200
Subject: [PATCH] Code review changes #1

---
 internals/overlord/checkstate/manager.go      | 17 +++++++----------
 internals/overlord/checkstate/manager_test.go |  4 ++--
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/internals/overlord/checkstate/manager.go b/internals/overlord/checkstate/manager.go
index af8db46f..6cba21ca 100644
--- a/internals/overlord/checkstate/manager.go
+++ b/internals/overlord/checkstate/manager.go
@@ -27,7 +27,7 @@ import (
 // CheckManager starts and manages the health checks.
 type CheckManager struct {
 	mutex           sync.Mutex
-	group           sync.WaitGroup
+	wg              sync.WaitGroup
 	checks          map[string]*checkData
 	failureHandlers []FailureFunc
 }
@@ -61,10 +61,10 @@ func (m *CheckManager) PlanChanged(p *plan.Plan) {
 	}
 	// Wait for all context cancellations to propagate and allow
 	// each goroutine to cleanly exit.
-	m.group.Wait()
+	m.wg.Wait()
 
 	// Set the size of the next wait group
-	m.group.Add(len(p.Checks))
+	m.wg.Add(len(p.Checks))
 
 	// Then configure and start new checks.
 	checks := make(map[string]*checkData, len(p.Checks))
@@ -72,14 +72,16 @@ func (m *CheckManager) PlanChanged(p *plan.Plan) {
 		ctx, cancel := context.WithCancel(context.Background())
 		check := &checkData{
 			config:  config,
-			group:   &m.group,
 			checker: newChecker(config),
 			ctx:     ctx,
 			cancel:  cancel,
 			action:  m.callFailureHandlers,
 		}
 		checks[name] = check
-		go check.loop()
+		go func() {
+			defer m.wg.Done()
+			check.loop()
+		}()
 	}
 	m.checks = checks
 }
@@ -163,7 +165,6 @@ const (
 // checkData holds state for an active health check.
 type checkData struct {
 	config  *plan.Check
-	group   *sync.WaitGroup
 	checker checker
 	ctx     context.Context
 	cancel  context.CancelFunc
@@ -180,10 +181,6 @@ type checker interface {
 }
 
 func (c *checkData) loop() {
-	// Schedule a notification on exit to indicate another
-	// checker in the group is complete.
-	defer c.group.Done()
-
 	logger.Debugf("Check %q starting with period %v", c.config.Name, c.config.Period.Value)
 
 	ticker := time.NewTicker(c.config.Period.Value)
diff --git a/internals/overlord/checkstate/manager_test.go b/internals/overlord/checkstate/manager_test.go
index 7388aa60..12b418b1 100644
--- a/internals/overlord/checkstate/manager_test.go
+++ b/internals/overlord/checkstate/manager_test.go
@@ -269,13 +269,13 @@ func (s *ManagerSuite) TestFailures(c *C) {
 
 // waitCheck is a time based approach to wait for a checker run to complete.
 // The timeout value does not impact the general time it takes for tests to
-// complete, but determines a worse case waiting period before giving up.
+// complete, but determines a worst case waiting period before giving up.
 // The timeout value must take into account single core or very busy machines
 // so it makes sense to pick a conservative number here as failing a test
 // due to a busy test resource is more extensive than waiting a few more
 // seconds.
 func waitCheck(c *C, mgr *CheckManager, name string, f func(check *CheckInfo) bool) *CheckInfo {
-	// Worse case waiting time for checker run(s) to complete. This
+	// Worst case waiting time for checker run(s) to complete. This
 	// period should be much longer (10x is good) than the longest
 	// check timeout value.
 	timeout := time.Second * 10