fix data race on loading config by multiple goroutines (fix #333)

Author: rhysd

config.go

@@ -3,6 +3,7 @@ package actionlint
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 
 	"gopkg.in/yaml.v3"
@@ -41,6 +42,24 @@ func ReadConfigFile(path string) (*Config, error) {
 	return parseConfig(b, path)
 }
 
+// loadRepoConfig reads config file from the repository's .github/actionlint.yml or
+// .github/actionlint.yaml.
+func loadRepoConfig(root string) (*Config, error) {
+	for _, f := range []string{"actionlint.yaml", "actionlint.yml"} {
+		path := filepath.Join(root, ".github", f)
+		b, err := os.ReadFile(path)
+		if err != nil {
+			continue // file does not exist
+		}
+		cfg, err := parseConfig(b, path)
+		if err != nil {
+			return nil, err
+		}
+		return cfg, nil
+	}
+	return nil, nil
+}
+
 func writeDefaultConfigFile(path string) error {
 	b := []byte(`self-hosted-runner:
   # Labels of self-hosted runner in array of strings.

linter.go

@@ -216,7 +216,10 @@ func (l *Linter) debugWriter() io.Writer {
 func (l *Linter) GenerateDefaultConfig(dir string) error {
 	l.log("Generating default actionlint.yaml in repository:", dir)
 
-	p := l.projects.At(dir)
+	p, err := l.projects.At(dir)
+	if err != nil {
+		return err
+	}
 	if p == nil {
 		return errors.New("project is not found. check current project is initialized as Git repository and \".github/workflows\" directory exists")
 	}
@@ -240,14 +243,17 @@ func (l *Linter) GenerateDefaultConfig(dir string) error {
 func (l *Linter) LintRepository(dir string) ([]*Error, error) {
 	l.log("Linting all workflow files in repository:", dir)
 
-	proj := l.projects.At(dir)
-	if proj == nil {
+	p, err := l.projects.At(dir)
+	if err != nil {
+		return nil, err
+	}
+	if p == nil {
 		return nil, fmt.Errorf("no project was found in any parent directories of %q. check workflows directory is put correctly in your Git repository", dir)
 	}
 
-	l.log("Detected project:", proj.RootDir())
-	wd := proj.WorkflowsDir()
-	return l.LintDir(wd, proj)
+	l.log("Detected project:", p.RootDir())
+	wd := p.WorkflowsDir()
+	return l.LintDir(wd, p)
 }
 
 // LintDir lints all YAML workflow files in the given directory recursively.
@@ -316,14 +322,18 @@ func (l *Linter) LintFiles(filepaths []string, project *Project) ([]*Error, erro
 	for i := range ws {
 		// Each element of ws is accessed by single goroutine so mutex is unnecessary
 		w := &ws[i]
-		p := project
-		if p == nil {
+		proj := project
+		if proj == nil {
 			// This method modifies state of l.projects so it cannot be called in parallel.
 			// Before entering goroutine, resolve project instance.
-			p = l.projects.At(w.path)
+			p, err := l.projects.At(w.path)
+			if err != nil {
+				return nil, err
+			}
+			proj = p
 		}
-		ac := acf.GetCache(p) // #173
-		rwc := rwcf.GetCache(p)
+		ac := acf.GetCache(proj) // #173
+		rwc := rwcf.GetCache(proj)
 
 		eg.Go(func() error {
 			// Bound concurrency on reading files to avoid "too many files to open" error (issue #3)
@@ -339,7 +349,7 @@ func (l *Linter) LintFiles(filepaths []string, project *Project) ([]*Error, erro
 					w.path = r // Use relative path if possible
 				}
 			}
-			errs, err := l.check(w.path, src, p, proc, ac, rwc)
+			errs, err := l.check(w.path, src, proj, proc, ac, rwc)
 			if err != nil {
 				return fmt.Errorf("fatal error while checking %s: %w", w.path, err)
 			}
@@ -389,7 +399,11 @@ func (l *Linter) LintFiles(filepaths []string, project *Project) ([]*Error, erro
 // parameter can be nil. In the case, the project is detected from the given path.
 func (l *Linter) LintFile(path string, project *Project) ([]*Error, error) {
 	if project == nil {
-		project = l.projects.At(path)
+		p, err := l.projects.At(path)
+		if err != nil {
+			return nil, err
+		}
+		project = p
 	}
 
 	src, err := os.ReadFile(path)
@@ -428,7 +442,11 @@ func (l *Linter) LintFile(path string, project *Project) ([]*Error, error) {
 func (l *Linter) Lint(path string, content []byte, project *Project) ([]*Error, error) {
 	if project == nil && path != "<stdin>" {
 		if _, err := os.Stat(path); !errors.Is(err, fs.ErrNotExist) {
-			project = l.projects.At(path)
+			p, err := l.projects.At(path)
+			if err != nil {
+				return nil, err
+			}
+			project = p
 		}
 	}
 	proc := newConcurrentProcess(runtime.NumCPU())
@@ -471,13 +489,10 @@ func (l *Linter) check(
 
 	var cfg *Config
 	if l.defaultConfig != nil {
+		// `-config-file` option has higher prioritiy than repository config file
 		cfg = l.defaultConfig
 	} else if project != nil {
-		c, err := project.LoadConfig()
-		if err != nil {
-			return nil, err
-		}
-		cfg = c
+		cfg = project.Config()
 	}
 	if cfg != nil {
 		l.debug("Config: %#v", cfg)

project.go

@@ -21,20 +21,24 @@ func absPath(path string) string {
 
 // findProject creates new Project instance by finding a project which the given path belongs to.
 // A project must be a Git repository and have ".github/workflows" directory.
-func findProject(path string) *Project {
+func findProject(path string) (*Project, error) {
 	d := absPath(path)
 	for {
 		w := filepath.Join(d, ".github", "workflows")
 		if s, err := os.Stat(w); err == nil && s.IsDir() {
 			g := filepath.Join(d, ".git")
 			if _, err := os.Stat(g); err == nil { // Note: .git may be a file
-				return &Project{root: d}
+				c, err := loadRepoConfig(d)
+				if err != nil {
+					return nil, err
+				}
+				return &Project{root: d, config: c}, nil
 			}
 		}
 
 		p := filepath.Dir(d)
 		if p == d {
-			return nil
+			return nil, nil
 		}
 		d = p
 	}
@@ -58,28 +62,12 @@ func (p *Project) Knows(path string) bool {
 	return strings.HasPrefix(absPath(path), p.root)
 }
 
-// LoadConfig returns config object of the GitHub project repository. The config file is read from
-// ".github/actionlint.yaml" or ".github/actionlint.yml".
-func (p *Project) LoadConfig() (*Config, error) {
-	if p.config != nil {
-		return p.config, nil
-	}
-
-	for _, f := range []string{"actionlint.yaml", "actionlint.yml"} {
-		path := filepath.Join(p.root, ".github", f)
-		b, err := os.ReadFile(path)
-		if err != nil {
-			continue // file does not exist
-		}
-		cfg, err := parseConfig(b, path)
-		if err != nil {
-			return nil, err
-		}
-		p.config = cfg
-		return cfg, nil
-	}
-
-	return nil, nil // not found
+// Config returns config object of the GitHub project repository. The config file was read from
+// ".github/actionlint.yaml" or ".github/actionlint.yml" when this Project instance was created.
+// When no config was found, this method returns nil.
+func (p *Project) Config() *Config {
+	// Note: Calling this method must be thread safe (#333)
+	return p.config
 }
 
 // Projects represents set of projects. It caches Project instances which was created previously
@@ -95,17 +83,20 @@ func NewProjects() *Projects {
 
 // At returns the Project instance which the path belongs to. It returns nil if no project is found
 // from the path.
-func (ps *Projects) At(path string) *Project {
+func (ps *Projects) At(path string) (*Project, error) {
 	for _, p := range ps.known {
 		if p.Knows(path) {
-			return p
+			return p, nil
 		}
 	}
 
-	p := findProject(path)
+	p, err := findProject(path)
+	if err != nil {
+		return nil, err
+	}
 	if p != nil {
 		ps.known = append(ps.known, p)
 	}
 
-	return p
+	return p, nil
 }

project_test.go

@@ -3,6 +3,7 @@ package actionlint
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 
@@ -57,15 +58,21 @@ func TestProjectsFindProjectFromPath(t *testing.T) {
 		},
 	} {
 		t.Run(tc.what, func(t *testing.T) {
-			p := ps.At(tc.path)
+			p, err := ps.At(tc.path)
+			if err != nil {
+				t.Fatal(err)
+			}
 
 			r := p.RootDir()
 			if r != abs {
 				t.Fatalf("root directory of project %v should be %q but got %q", p, abs, r)
 			}
 
 			// Result should be cached
-			p2 := ps.At(tc.path)
+			p2, err := ps.At(tc.path)
+			if err != nil {
+				t.Fatal(err)
+			}
 			if p != p2 {
 				t.Fatalf("project %v is not cached. New project is %v. %p v.s. %p", p, p2, p, p2)
 			}
@@ -83,8 +90,60 @@ func TestProjectsDoesNotFindProjectFromOutside(t *testing.T) {
 
 	outside := filepath.Join(d, "..")
 	ps := NewProjects()
-	p := ps.At(outside)
+	p, err := ps.At(outside)
+	if err != nil {
+		t.Fatal(err)
+	}
 	if p != nil && p.RootDir() == abs {
 		t.Fatalf("project %v is detected from outside of the project %q", p, outside)
 	}
 }
+
+func TestProjectsLoadingProjectConfig(t *testing.T) {
+	d := filepath.Join("testdata", "config", "projects", "ok")
+	testEnsureDotGitDir(d)
+	ps := NewProjects()
+	p, err := ps.At(d)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p == nil {
+		t.Fatal("project was not found at", d)
+	}
+	if c := p.Config(); c == nil {
+		t.Fatal("config was not found for directory", d)
+	}
+}
+
+func TestProjectsLoadingNoProjectConfig(t *testing.T) {
+	d := filepath.Join("testdata", "config", "projects", "none")
+	testEnsureDotGitDir(d)
+	ps := NewProjects()
+	p, err := ps.At(d)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p == nil {
+		t.Fatal("project was not found at", d)
+	}
+	if c := p.Config(); c != nil {
+		t.Fatal("config was found for directory", d)
+	}
+}
+
+func TestProjectsLoadingBrokenProjectConfig(t *testing.T) {
+	want := "could not parse config file"
+	d := filepath.Join("testdata", "config", "projects", "err")
+	testEnsureDotGitDir(d)
+	ps := NewProjects()
+	p, err := ps.At(d)
+	if err == nil {
+		t.Fatalf("wanted error %q but have no error", want)
+	}
+	if p != nil {
+		t.Fatal("project was returned though getting config failed", p)
+	}
+	if msg := err.Error(); !strings.Contains(msg, want) {
+		t.Fatalf("wanted error %q but have error %q", want, msg)
+	}
+}

testdata/config/projects/err/.github/actionlint.yaml

@@ -0,0 +1 @@
+self-hosted-runner: 42

testdata/config/projects/err/.github/workflows/test.yaml

@@ -0,0 +1,6 @@
+on: push
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo

testdata/config/projects/none/.github/workflows/test.yaml

@@ -0,0 +1,6 @@
+on: push
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo

testdata/config/projects/ok/.github/actionlint.yaml

@@ -0,0 +1,3 @@
+self-hosted-runner:
+  labels: []
+config-variables: null

testdata/config/projects/ok/.github/workflows/test.yaml

@@ -0,0 +1,6 @@
+on: push
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo