Skip to content

Commit

Permalink
Make GetRunKey threadsafe 🔒
Browse files Browse the repository at this point in the history
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 tektoncd#1124
  • Loading branch information
bobcatfish committed Sep 13, 2019
1 parent 1c89311 commit 0e9a727
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 11 deletions.
3 changes: 2 additions & 1 deletion pkg/apis/pipeline/v1alpha1/pipelinerun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 3 additions & 7 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1alpha1_test

import (
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -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())
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/apis/pipeline/v1alpha1/taskrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
5 changes: 3 additions & 2 deletions pkg/apis/pipeline/v1alpha1/taskrun_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1alpha1_test

import (
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -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())
}
Expand Down

0 comments on commit 0e9a727

Please sign in to comment.