cmd/compile: add a test case and some comments for deadlock on syntax error

hopehook · gopherbot · commit 370cadd0e43d · 2022-04-12T01:18:27.000Z
After CL 398014 fixed a compiler deadlock on syntax errors, this CL adds a test case and more details for that. How it was fixed: CL 57751 introduced a channel "sem" to limit the number of simultaneously open files. Unfortunately, when the number of syntax processing goroutines exceeds this limit, will easily trigger deadlock. In the original implementation, "sem" only limited the number of open files, not the number of concurrent goroutines, which will cause extra goroutines to block on "sem". When the p.err of the following iteration happens to be held by the blocking goroutine, it will fall into a circular wait, which is a deadlock. CL 398014 fixed the above deadlock, also see issue golang#52127. First, move "sem <- struct{}{}" to the outside of the syntax processing goroutine, so that the number of concurrent goroutines does not exceed the number of open files, to ensure that all goroutines in execution can eventually write to p.err. Second, move the entire syntax processing logic into a separate goroutine to avoid blocking on the producer side. Change-Id: I1bb89bfee3d2703784f0c0d4ded82baab2ae867a Reviewed-on: https://go-review.googlesource.com/c/go/+/399054 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Auto-Submit: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com>
diff --git a/src/cmd/compile/internal/noder/noder.go b/src/cmd/compile/internal/noder/noder.go
@@ -40,6 +40,7 @@ func LoadPackage(filenames []string) {
 		noders[i] = &p
 	}
 
+	// Move the entire syntax processing logic into a separate goroutine to avoid blocking on the "sem".
 	go func() {
 		for i, filename := range filenames {
 			filename := filename
diff --git a/test/fixedbugs/issue52127.go b/test/fixedbugs/issue52127.go
@@ -0,0 +1,62 @@
+// run
+//go:build !js
+// +build !js
+
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Issue 52127: Too many syntax errors in many files can
+// cause deadlocks instead of displaying error messages
+// correctly.
+
+package main
+
+import (
+	"bytes"
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+)
+
+func main() {
+	dir, err := os.MkdirTemp("", "issue52127")
+	if err != nil {
+		panic(err)
+	}
+	defer os.RemoveAll(dir)
+
+	args := []string{"go", "build"}
+	write := func(prefix string, i int, data string) {
+		filename := filepath.Join(dir, fmt.Sprintf("%s%d.go", prefix, i))
+		if err := os.WriteFile(filename, []byte(data), 0o644); err != nil {
+			panic(err)
+		}
+		args = append(args, filename)
+	}
+
+	for i := 0; i < 100; i++ {
+		write("a", i, `package p
+`)
+	}
+	for i := 0; i < 100; i++ {
+		write("b", i, `package p
+var
+var
+var
+var
+var
+`)
+	}
+
+	cmd := exec.Command(args[0], args[1:]...)
+	output, err := cmd.CombinedOutput()
+	if err == nil {
+		panic("compile succeeded unexpectedly")
+	}
+	if !bytes.Contains(output, []byte("syntax error:")) {
+		panic(fmt.Sprintf(`missing "syntax error" in compiler output; got: 
+%s`, output))
+	}
+}

Original file line number	Diff line number	Diff line change
`@@ -40,6 +40,7 @@ func LoadPackage(filenames []string) {`
`40`	`40`	`noders[i] = &p`
`41`	`41`	`}`
`42`	`42`
	`43`	`+ // Move the entire syntax processing logic into a separate goroutine to avoid blocking on the "sem".`
`43`	`44`	`go func() {`
`44`	`45`	`for i, filename := range filenames {`
`45`	`46`	`filename := filename`