From 470f673a9e230d6b05f6a40ade98da0b527eebf0 Mon Sep 17 00:00:00 2001
From: hc-github-team-nomad-core
 <82989552+hc-github-team-nomad-core@users.noreply.github.com>
Date: Fri, 11 Nov 2022 11:02:48 -0500
Subject: [PATCH] client: avoid unconsumed channel in timer construction
 (#15215) (#15218)

* client: avoid unconsumed channel in timer construction

This PR fixes a bug introduced in #11983 where a Timer initialized with 0
duration causes an immediate tick, even if Reset is called before reading the
channel. The fix is to avoid doing that, instead creating a Timer with a non-zero
initial wait time, and then immediately calling Stop.

* pr: remove redundant stop

Co-authored-by: Seth Hoenig <shoenig@duck.com>
---
 .changelog/15215.txt                         |  3 ++
 client/allocrunner/taskrunner/task_runner.go |  3 +-
 helper/funcs.go                              | 23 ++++++++++++
 helper/funcs_test.go                         | 38 +++++++++++++++++++-
 4 files changed, 65 insertions(+), 2 deletions(-)
 create mode 100644 .changelog/15215.txt

diff --git a/.changelog/15215.txt b/.changelog/15215.txt
new file mode 100644
index 000000000000..4428ce62316e
--- /dev/null
+++ b/.changelog/15215.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+client: Fixed a bug where tasks would restart without waiting for interval
+```
diff --git a/client/allocrunner/taskrunner/task_runner.go b/client/allocrunner/taskrunner/task_runner.go
index 8202ebbac505..a77c6329a7c8 100644
--- a/client/allocrunner/taskrunner/task_runner.go
+++ b/client/allocrunner/taskrunner/task_runner.go
@@ -554,7 +554,8 @@ func (tr *TaskRunner) Run() {
 	// Set the initial task state.
 	tr.stateUpdater.TaskStateUpdated()
 
-	timer, stop := helper.NewSafeTimer(0) // timer duration calculated JIT
+	// start with a stopped timer; actual restart delay computed later
+	timer, stop := helper.NewStoppedTimer()
 	defer stop()
 
 MAIN:
diff --git a/helper/funcs.go b/helper/funcs.go
index 57335fea54d4..fb962e9bf51e 100644
--- a/helper/funcs.go
+++ b/helper/funcs.go
@@ -3,6 +3,7 @@ package helper
 import (
 	"crypto/sha512"
 	"fmt"
+	"math"
 	"net/http"
 	"path/filepath"
 	"reflect"
@@ -368,6 +369,9 @@ type StopFunc func()
 //
 // Returns the time.Timer and also a StopFunc, forcing the caller to deal
 // with stopping the time.Timer to avoid leaking a goroutine.
+//
+// Note: If creating a Timer that should do nothing until Reset is called, use
+// NewStoppedTimer instead for safely creating the timer in a stopped state.
 func NewSafeTimer(duration time.Duration) (*time.Timer, StopFunc) {
 	if duration <= 0 {
 		// Avoid panic by using the smallest positive value. This is close enough
@@ -385,6 +389,25 @@ func NewSafeTimer(duration time.Duration) (*time.Timer, StopFunc) {
 	return t, cancel
 }
 
+// NewStoppedTimer creates a time.Timer in a stopped state. This is useful when
+// the actual wait time will computed and set later via Reset.
+func NewStoppedTimer() (*time.Timer, StopFunc) {
+	t, f := NewSafeTimer(math.MaxInt64)
+	t.Stop()
+	return t, f
+}
+
+// ConvertSlice takes the input slice and generates a new one using the
+// supplied conversion function to covert the element. This is useful when
+// converting a slice of strings to a slice of structs which wraps the string.
+func ConvertSlice[A, B any](original []A, conversion func(a A) B) []B {
+	result := make([]B, len(original))
+	for i, element := range original {
+		result[i] = conversion(element)
+	}
+	return result
+}
+
 // IsMethodHTTP returns whether s is a known HTTP method, ignoring case.
 func IsMethodHTTP(s string) bool {
 	switch strings.ToUpper(s) {
diff --git a/helper/funcs_test.go b/helper/funcs_test.go
index 5c2ec62b6611..720c5d2dc51b 100644
--- a/helper/funcs_test.go
+++ b/helper/funcs_test.go
@@ -376,7 +376,7 @@ func TestCheckNamespaceScope(t *testing.T) {
 	}
 }
 
-func Test_NewSafeTimer(t *testing.T) {
+func TestTimer_NewSafeTimer(t *testing.T) {
 	t.Run("zero", func(t *testing.T) {
 		timer, stop := NewSafeTimer(0)
 		defer stop()
@@ -390,6 +390,42 @@ func Test_NewSafeTimer(t *testing.T) {
 	})
 }
 
+func TestTimer_NewStoppedTimer(t *testing.T) {
+	timer, stop := NewStoppedTimer()
+	defer stop()
+
+	select {
+	case <-timer.C:
+		must.Unreachable(t)
+	default:
+	}
+}
+
+func Test_ConvertSlice(t *testing.T) {
+	t.Run("string wrapper", func(t *testing.T) {
+
+		type wrapper struct{ id string }
+		input := []string{"foo", "bar", "bad", "had"}
+		cFn := func(id string) *wrapper { return &wrapper{id: id} }
+
+		expectedOutput := []*wrapper{{id: "foo"}, {id: "bar"}, {id: "bad"}, {id: "had"}}
+		actualOutput := ConvertSlice(input, cFn)
+		require.ElementsMatch(t, expectedOutput, actualOutput)
+	})
+
+	t.Run("int wrapper", func(t *testing.T) {
+
+		type wrapper struct{ id int }
+		input := []int{10, 13, 1987, 2020}
+		cFn := func(id int) *wrapper { return &wrapper{id: id} }
+
+		expectedOutput := []*wrapper{{id: 10}, {id: 13}, {id: 1987}, {id: 2020}}
+		actualOutput := ConvertSlice(input, cFn)
+		require.ElementsMatch(t, expectedOutput, actualOutput)
+
+	})
+}
+
 func Test_IsMethodHTTP(t *testing.T) {
 	t.Run("is method", func(t *testing.T) {
 		cases := []string{