fix race condition when process is created and proc.wait() is calle…

…d at the same time Note: The race condition issue occurred in this job: https://github.com/rhysd/actionlint/actions/runs/7942341834/job/21685544722 ``` WARNING: DATA RACE Write at 0x00c0001dcb00 by main goroutine: runtime.racewrite() <autogenerated>:1 +0x1e github.com/rhysd/actionlint.(*concurrentProcess).wait() /Users/runner/work/actionlint/actionlint/process.go:82 +0x134b github.com/rhysd/actionlint.(*Linter).LintFiles() /Users/runner/work/actionlint/actionlint/linter.go:362 +0x1346 github.com/rhysd/actionlint.(*Linter).LintDir() /Users/runner/work/actionlint/actionlint/linter.go:285 +0x26e github.com/rhysd/actionlint.(*Linter).LintRepository() /Users/runner/work/actionlint/actionlint/linter.go:256 +0x2b5 github.com/rhysd/actionlint.(*Command).runLinter() /Users/runner/work/actionlint/actionlint/command.go:91 +0x34b github.com/rhysd/actionlint.(*Command).Main() /Users/runner/work/actionlint/actionlint/command.go:180 +0xcad main.main() /Users/runner/work/actionlint/actionlint/cmd/actionlint/main.go:15 +0x124 Previous read at 0x00c0001dcb00 by goroutine 6: runtime.raceread() <autogenerated>:1 +0x1e github.com/rhysd/actionlint.(*concurrentProcess).run() /Users/runner/work/actionlint/actionlint/process.go:71 +0xab github.com/rhysd/actionlint.(*externalCommand).run() /Users/runner/work/actionlint/actionlint/process.go:113 +0x7cd github.com/rhysd/actionlint.(*RuleShellcheck).runShellcheck() /Users/runner/work/actionlint/actionlint/rule_shellcheck.go:186 +0x615 github.com/rhysd/actionlint.(*RuleShellcheck).VisitStep() /Users/runner/work/actionlint/actionlint/rule_shellcheck.go:60 +0x1ef github.com/rhysd/actionlint.(*Visitor).visitStep() /Users/runner/work/actionlint/actionlint/pass.go:139 +0x108 github.com/rhysd/actionlint.(*Visitor).visitJob() /Users/runner/work/actionlint/actionlint/pass.go:109 +0x2cd github.com/rhysd/actionlint.(*Visitor).Visit() /Users/runner/work/actionlint/actionlint/pass.go:68 +0x29c github.com/rhysd/actionlint.(*Linter).check() /Users/runner/work/actionlint/actionlint/linter.go:570 +0x20b9 github.com/rhysd/actionlint.(*Linter).LintFiles.func1() /Users/runner/work/actionlint/actionlint/linter.go:352 +0x246 golang.org/x/sync/errgroup.(*Group).Go.func1() /Users/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:78 +0x91 Goroutine 6 (running) created at: golang.org/x/sync/errgroup.(*Group).Go() /Users/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:75 +0x124 github.com/rhysd/actionlint.(*Linter).LintFiles() /Users/runner/work/actionlint/actionlint/linter.go:338 +0x79d github.com/rhysd/actionlint.(*Linter).LintDir() /Users/runner/work/actionlint/actionlint/linter.go:285 +0x26e github.com/rhysd/actionlint.(*Linter).LintRepository() /Users/runner/work/actionlint/actionlint/linter.go:256 +0x2b5 github.com/rhysd/actionlint.(*Command).runLinter() /Users/runner/work/actionlint/actionlint/command.go:91 +0x34b github.com/rhysd/actionlint.(*Command).Main() /Users/runner/work/actionlint/actionlint/command.go:180 +0xcad main.main() /Users/runner/work/actionlint/actionlint/cmd/actionlint/main.go:15 +0x124 ```
rhysd · Feb 18, 2024 · 9eb8b7b · 9eb8b7b
1 parent f93f97b
commit 9eb8b7b
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 3 deletions.
diff --git a/linter.go b/linter.go
@@ -300,8 +300,9 @@ func (l *Linter) LintFiles(filepaths []string, project *Project) ([]*Error, erro
 	l.log("Linting", n, "files")
 
 	cwd := l.cwd
-	proc := newConcurrentProcess(runtime.NumCPU())
-	sema := semaphore.NewWeighted(int64(runtime.NumCPU()))
+	cpus := runtime.NumCPU()
+	proc := newConcurrentProcess(cpus)
+	sema := semaphore.NewWeighted(int64(cpus))
 	ctx := context.Background()
 	dbg := l.debugWriter()
 	acf := NewLocalActionsCacheFactory(dbg)
@@ -359,11 +360,18 @@ func (l *Linter) LintFiles(filepaths []string, project *Project) ([]*Error, erro
 		})
 	}
 
-	proc.wait()
 	if err := eg.Wait(); err != nil {
 		return nil, err
 	}
 
+	// Ensure that all processes finish. `proc.wait()` must be called after `eg.Wait()`.
+	// Calling `WaitGroup.Add` after `WaitGroup.Wait` can cause a race condition (specifically when
+	// increasing the group count from 0 to 1 and calling `Wait` and at the same time).
+	// `WaitGroup.Add` is called in `proc.run()` and `WaitGroup.Wait` is called in `proc.wait()`.
+	// After traversing all workflows, `proc.run()` is no longer called so `proc.wait()` can be
+	// called safely.
+	proc.wait()
+
 	total := 0
 	for i := range ws {
 		total += len(ws[i].errs)

diff --git a/process_test.go b/process_test.go
@@ -290,3 +290,20 @@ func TestProcessErrorLinterFailed(t *testing.T) {
 		t.Fatal("a command following the error did not run")
 	}
 }
+
+func TestProcessRunConcurrentlyAndWait(t *testing.T) {
+	p := newConcurrentProcess(2)
+	ls := testSkipIfNoCommand(t, p, "ls")
+
+	c := make(chan struct{})
+	go func() {
+		for i := 0; i < 5; i++ {
+			ls.run(nil, "", func(b []byte, err error) error {
+				return err
+			})
+		}
+		c <- struct{}{}
+	}()
+	<-c
+	p.wait()
+}
diff --git a/rule_shellcheck.go b/rule_shellcheck.go
@@ -197,6 +197,7 @@ func (rule *RuleShellcheck) runShellcheck(src, sh string, pos *Pos) {
 			return nil
 		}
 
+		// Synchronize rule.Errorf calls
 		rule.mu.Lock()
 		defer rule.mu.Unlock()
 		// It's better to show source location in the script as position of error, but it's not