mirror of
https://code.forgejo.org/forgejo/runner.git
synced 2025-09-05 18:40:59 +00:00
chore: prevent 'false positive' data race detection with Job.If [skip cascade] (#864)
Initialization of the default value of the `If` field is functionally safe, but triggers the data race detector. ``` ================== WARNING: DATA RACE Read at 0x00c00037cf78 by goroutine 10: code.forgejo.org/forgejo/runner/v9/act/model.(*Workflow).GetJob() /.../forgejo-runner/act/model/workflow.go:766 +0x2ae code.forgejo.org/forgejo/runner/v9/act/model.(*Run).Job() /.../forgejo-runner/act/model/planner.go:50 +0xab code.forgejo.org/forgejo/runner/v9/act/runner.setJobResult() /.../forgejo-runner/act/runner/job_executor.go:168 +0x7c code.forgejo.org/forgejo/runner/v9/act/runner.TestSetJobResultConcurrency.func3() /.../forgejo-runner/act/runner/job_executor_test.go:410 +0xf8 Previous write at 0x00c00037cf78 by goroutine 9: code.forgejo.org/forgejo/runner/v9/act/model.(*Workflow).GetJob() /.../forgejo-runner/act/model/workflow.go:767 +0x2ce code.forgejo.org/forgejo/runner/v9/act/model.(*Run).Job() /.../forgejo-runner/act/model/planner.go:50 +0xab code.forgejo.org/forgejo/runner/v9/act/runner.setJobResult() /.../forgejo-runner/act/runner/job_executor.go:168 +0x7c code.forgejo.org/forgejo/runner/v9/act/runner.TestSetJobResultConcurrency.func2() /.../forgejo-runner/act/runner/job_executor_test.go:405 +0xfb Goroutine 10 (running) created at: code.forgejo.org/forgejo/runner/v9/act/runner.TestSetJobResultConcurrency() /.../forgejo-runner/act/runner/job_executor_test.go:408 +0xbac testing.tRunner() /home/mfenniak/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1792 +0x225 testing.(*T).Run.gowrap1() /home/mfenniak/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1851 +0x44 Goroutine 9 (running) created at: code.forgejo.org/forgejo/runner/v9/act/runner.TestSetJobResultConcurrency() /.../forgejo-runner/act/runner/job_executor_test.go:403 +0xa84 testing.tRunner() /home/mfenniak/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1792 +0x225 testing.(*T).Run.gowrap1() /home/mfenniak/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1851 +0x44 ================== ``` <!--start release-notes-assistant--> <!--URL:https://code.forgejo.org/forgejo/runner--> - other - [PR](https://code.forgejo.org/forgejo/runner/pulls/864): <!--number 864 --><!--line 0 --><!--description Y2hvcmU6IHByZXZlbnQgJ2ZhbHNlIHBvc2l0aXZlJyBkYXRhIHJhY2UgZGV0ZWN0aW9uIHdpdGggSm9iLklmIFtza2lwIGNhc2NhZGVd-->chore: prevent 'false positive' data race detection with Job.If [skip cascade]<!--description--> <!--end release-notes-assistant--> Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/864 Reviewed-by: earl-warren <earl-warren@noreply.code.forgejo.org> Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
parent
69a3adad21
commit
d63d70c080
3 changed files with 12 additions and 8 deletions
|
@ -207,7 +207,7 @@ type Job struct {
|
||||||
RawNeeds yaml.Node `yaml:"needs"`
|
RawNeeds yaml.Node `yaml:"needs"`
|
||||||
RawRunsOn yaml.Node `yaml:"runs-on"`
|
RawRunsOn yaml.Node `yaml:"runs-on"`
|
||||||
Env yaml.Node `yaml:"env"`
|
Env yaml.Node `yaml:"env"`
|
||||||
If yaml.Node `yaml:"if"`
|
RawIf yaml.Node `yaml:"if"`
|
||||||
Steps []*Step `yaml:"steps"`
|
Steps []*Step `yaml:"steps"`
|
||||||
TimeoutMinutes string `yaml:"timeout-minutes"`
|
TimeoutMinutes string `yaml:"timeout-minutes"`
|
||||||
Services map[string]*ContainerSpec `yaml:"services"`
|
Services map[string]*ContainerSpec `yaml:"services"`
|
||||||
|
@ -362,6 +362,13 @@ func (j *Job) RunsOn() []string {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (j *Job) IfClause() string {
|
||||||
|
if j.RawIf.Value == "" {
|
||||||
|
return "success()"
|
||||||
|
}
|
||||||
|
return j.RawIf.Value
|
||||||
|
}
|
||||||
|
|
||||||
func nodeAsStringSlice(node yaml.Node) []string {
|
func nodeAsStringSlice(node yaml.Node) []string {
|
||||||
switch node.Kind {
|
switch node.Kind {
|
||||||
case yaml.ScalarNode:
|
case yaml.ScalarNode:
|
||||||
|
@ -761,9 +768,6 @@ func (w *Workflow) GetJob(jobID string) *Job {
|
||||||
if j.Name == "" {
|
if j.Name == "" {
|
||||||
j.Name = id
|
j.Name = id
|
||||||
}
|
}
|
||||||
if j.If.Value == "" {
|
|
||||||
j.If.Value = "success()"
|
|
||||||
}
|
|
||||||
return j
|
return j
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
@ -1022,11 +1022,11 @@ func (rc *RunContext) options(ctx context.Context) string {
|
||||||
func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) {
|
func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) {
|
||||||
job := rc.Run.Job()
|
job := rc.Run.Job()
|
||||||
l := common.Logger(ctx)
|
l := common.Logger(ctx)
|
||||||
runJob, runJobErr := EvalBool(ctx, rc.ExprEval, job.If.Value, exprparser.DefaultStatusCheckSuccess)
|
runJob, runJobErr := EvalBool(ctx, rc.ExprEval, job.IfClause(), exprparser.DefaultStatusCheckSuccess)
|
||||||
jobType, jobTypeErr := job.Type()
|
jobType, jobTypeErr := job.Type()
|
||||||
|
|
||||||
if runJobErr != nil {
|
if runJobErr != nil {
|
||||||
return false, fmt.Errorf(" \u274C Error in if-expression: \"if: %s\" (%s)", job.If.Value, runJobErr)
|
return false, fmt.Errorf(" \u274C Error in if-expression: \"if: %s\" (%s)", job.IfClause(), runJobErr)
|
||||||
}
|
}
|
||||||
|
|
||||||
if jobType == model.JobTypeInvalid {
|
if jobType == model.JobTypeInvalid {
|
||||||
|
@ -1035,7 +1035,7 @@ func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) {
|
||||||
|
|
||||||
if !runJob {
|
if !runJob {
|
||||||
rc.result("skipped")
|
rc.result("skipped")
|
||||||
l.WithField("jobResult", "skipped").Infof("Skipping job '%s' due to '%s'", job.Name, job.If.Value)
|
l.WithField("jobResult", "skipped").Infof("Skipping job '%s' due to '%s'", job.Name, job.IfClause())
|
||||||
return false, nil
|
return false, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -154,7 +154,7 @@ func (runner *runnerImpl) NewPlanExecutor(plan *model.Plan) common.Executor {
|
||||||
log.Debugf("Job.RawNeeds: %v", job.RawNeeds)
|
log.Debugf("Job.RawNeeds: %v", job.RawNeeds)
|
||||||
log.Debugf("Job.RawRunsOn: %v", job.RawRunsOn)
|
log.Debugf("Job.RawRunsOn: %v", job.RawRunsOn)
|
||||||
log.Debugf("Job.Env: %v", job.Env)
|
log.Debugf("Job.Env: %v", job.Env)
|
||||||
log.Debugf("Job.If: %v", job.If)
|
log.Debugf("Job.If: %v", job.IfClause())
|
||||||
for step := range job.Steps {
|
for step := range job.Steps {
|
||||||
if nil != job.Steps[step] {
|
if nil != job.Steps[step] {
|
||||||
log.Debugf("Job.Steps: %v", job.Steps[step].String())
|
log.Debugf("Job.Steps: %v", job.Steps[step].String())
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue