move mutex to Job
Makes more sense since it is the data object being modified, and although the synthetic test was passing in prev commit the actual tests w/ data race detector were still tripping. This suggests multiple RunContext objects are in-use with the same Job, which the test has been updated to represent.
This commit is contained in:
parent
c55251bbd5
commit
7242b248b0
4 changed files with 25 additions and 13 deletions
|
|
@ -11,6 +11,7 @@ import (
|
|||
"slices"
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
|
||||
"code.forgejo.org/forgejo/runner/v9/act/common"
|
||||
"code.forgejo.org/forgejo/runner/v9/act/schema"
|
||||
|
|
@ -217,7 +218,9 @@ type Job struct {
|
|||
Uses string `yaml:"uses"`
|
||||
With map[string]any `yaml:"with"`
|
||||
RawSecrets yaml.Node `yaml:"secrets"`
|
||||
Result string
|
||||
|
||||
Result string
|
||||
ResultMutex sync.Mutex
|
||||
}
|
||||
|
||||
// Strategy for the job
|
||||
|
|
|
|||
|
|
@ -164,8 +164,8 @@ func setJobResult(ctx context.Context, info jobInfo, rc *RunContext, success boo
|
|||
|
||||
// As we're reading the matrix build's status (`rc.Run.Job().Result`), it's possible for it change in another
|
||||
// goroutine running `setJobResult` and invoking `.result(...)`. Prevent concurrent execution of `setJobResult`...
|
||||
rc.JobResultMutex.Lock()
|
||||
defer rc.JobResultMutex.Unlock()
|
||||
rc.Run.Job().ResultMutex.Lock()
|
||||
defer rc.Run.Job().ResultMutex.Unlock()
|
||||
|
||||
jobResult := "success"
|
||||
// we have only one result for a whole matrix build, so we need
|
||||
|
|
|
|||
|
|
@ -338,14 +338,26 @@ func TestJobExecutorNewJobExecutor(t *testing.T) {
|
|||
|
||||
func TestSetJobResultConcurrency(t *testing.T) {
|
||||
jim := &jobInfoMock{}
|
||||
rc := &RunContext{
|
||||
job := model.Job{
|
||||
Result: "success",
|
||||
}
|
||||
// Distinct RunContext objects are used to replicate realistic setJobResult in matrix build
|
||||
rc1 := &RunContext{
|
||||
Run: &model.Run{
|
||||
JobID: "test",
|
||||
Workflow: &model.Workflow{
|
||||
Jobs: map[string]*model.Job{
|
||||
"test": {
|
||||
Result: "success",
|
||||
},
|
||||
"test": &job,
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
rc2 := &RunContext{
|
||||
Run: &model.Run{
|
||||
JobID: "test",
|
||||
Workflow: &model.Workflow{
|
||||
Jobs: map[string]*model.Job{
|
||||
"test": &job,
|
||||
},
|
||||
},
|
||||
},
|
||||
|
|
@ -374,7 +386,7 @@ func TestSetJobResultConcurrency(t *testing.T) {
|
|||
if result == "success" {
|
||||
time.Sleep(1 * time.Second)
|
||||
}
|
||||
rc.Run.Job().Result = result
|
||||
job.Result = result
|
||||
lastResult = result
|
||||
})
|
||||
|
||||
|
|
@ -383,12 +395,12 @@ func TestSetJobResultConcurrency(t *testing.T) {
|
|||
// Goroutine 1, mark as success:
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
setJobResult(t.Context(), jim, rc, true)
|
||||
setJobResult(t.Context(), jim, rc1, true)
|
||||
}()
|
||||
// Goroutine 2, mark as failure:
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
setJobResult(t.Context(), jim, rc, false)
|
||||
setJobResult(t.Context(), jim, rc2, false)
|
||||
}()
|
||||
wg.Wait()
|
||||
|
||||
|
|
|
|||
|
|
@ -18,7 +18,6 @@ import (
|
|||
"regexp"
|
||||
"runtime"
|
||||
"strings"
|
||||
"sync"
|
||||
"text/template"
|
||||
"time"
|
||||
|
||||
|
|
@ -57,8 +56,6 @@ type RunContext struct {
|
|||
randomName string
|
||||
networkName string
|
||||
networkCreated bool
|
||||
|
||||
JobResultMutex sync.Mutex
|
||||
}
|
||||
|
||||
func (rc *RunContext) AddMask(mask string) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue