From 10b64272fd2fabbf022acd634d4ddb45866528d2 Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Fri, 13 Sep 2019 14:53:35 -0400 Subject: [PATCH] =?UTF-8?q?Make=20GetRunKey=20threadsafe=20=F0=9F=94=92?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetRunKey is accessed in goroutines via the timeout_handler; accessing attributes of an object in a goroutine is not threadsafe. This is used as a key in a map, so for now replacing this with a value that should be unique but also threadsafe to fix #1124 --- pkg/apis/pipeline/v1alpha1/pipelinerun_types.go | 3 ++- pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go | 10 +++------- pkg/apis/pipeline/v1alpha1/taskrun_types.go | 3 ++- pkg/apis/pipeline/v1alpha1/taskrun_types_test.go | 5 +++-- 4 files changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go index b6cef00be9d..7b740c82fac 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go @@ -246,7 +246,8 @@ func (pr *PipelineRun) IsCancelled() bool { // GetRunKey return the pipelinerun key for timeout handler map func (pr *PipelineRun) GetRunKey() string { - return fmt.Sprintf("%s/%s/%s", pipelineRunControllerName, pr.Namespace, pr.Name) + // The address of the pointer is a threadsafe unique identifier for the pipelinerun + return fmt.Sprintf("%s/%p", pipelineRunControllerName, pr) } // IsTimedOut returns true if a pipelinerun has exceeded its spec.Timeout based on its status.Timeout diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go index 83ed60cb75c..6df40728fc5 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go @@ -17,6 +17,7 @@ limitations under the License. package v1alpha1_test import ( + "fmt" "testing" "time" @@ -130,13 +131,8 @@ func TestPipelineRunIsCancelled(t *testing.T) { } func TestPipelineRunKey(t *testing.T) { - pr := &v1alpha1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "prunname", - Namespace: "testns", - }, - } - expectedKey := "PipelineRun/testns/prunname" + pr := tb.PipelineRun("prunname", "testns") + expectedKey := fmt.Sprintf("PipelineRun/%p", pr) if pr.GetRunKey() != expectedKey { t.Fatalf("Expected taskrun key to be %s but got %s", expectedKey, pr.GetRunKey()) } diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_types.go b/pkg/apis/pipeline/v1alpha1/taskrun_types.go index ae60cfd2049..1273e72f6dd 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_types.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_types.go @@ -291,5 +291,6 @@ func (tr *TaskRun) IsCancelled() bool { // GetRunKey return the taskrun key for timeout handler map func (tr *TaskRun) GetRunKey() string { - return fmt.Sprintf("%s/%s/%s", "TaskRun", tr.Namespace, tr.Name) + // The address of the pointer is a threadsafe unique identifier for the taskrun + return fmt.Sprintf("%s/%p", "TaskRun", tr) } diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_types_test.go b/pkg/apis/pipeline/v1alpha1/taskrun_types_test.go index 0b0e46ea942..fa535144743 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_types_test.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_types_test.go @@ -17,6 +17,7 @@ limitations under the License. package v1alpha1_test import ( + "fmt" "testing" "time" @@ -112,8 +113,8 @@ func TestTaskRunIsCancelled(t *testing.T) { } func TestTaskRunKey(t *testing.T) { - tr := tb.TaskRun("taskrunname", "testns") - expectedKey := "TaskRun/testns/taskrunname" + tr := tb.TaskRun("taskrunname", "") + expectedKey := fmt.Sprintf("TaskRun/%p", tr) if tr.GetRunKey() != expectedKey { t.Fatalf("Expected taskrun key to be %s but got %s", expectedKey, tr.GetRunKey()) }