Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
overhaul how we load module info to format Go files
We would call "go mod edit -json" for each Go file we formatted, as each file may be in a different directory, and thus inside a different module. However, the first mistake is that we always ran the command in the directory where gofumpt is run, not the directory containing each Go file. Our tests weren't strict enough to catch this; now octal-literal.txt is, via its run of gofmt before it calls cd. The second mistake, and a pretty embarrassing one, is that since v0.3.0 made gofumpt concurrent, it has been racy in how it writes to globals like langVersion from multiple goroutines. octal-literals.txt now tests for this by adding a nested Go module. Finally, we could also run into open file limits, because spawning a child process and grabbing its output opens files of its own such as named pipes. The added test shows this with a limit of 256 and 10k tiny Go files: --- FAIL: TestWithLowOpenFileLimit (0.30s) ulimit_unix_test.go:82: writing 10000 tiny Go files ulimit_unix_test.go:104: running with 1 paths ulimit_unix_test.go:104: running with 10000 paths ulimit_unix_test.go:112: error: got non-nil error comment: stderr: open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/014.go: too many open files open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/017.go: too many open files open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/000.go: too many open files open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/019.go: too many open files Instead, only call "go mod edit -json" once per directory, and do it in the main thread to reduce its parallelism. Also make it grab fdSem as well, for good measure. This may not be a complete fix, as we're not sure how many files are open by an exec.Command.Output call. However, we are no longer able to reproduce a failure, so leave that as a TODO. Fixes #208.
- Loading branch information
Showing
4 changed files
with
193 additions
and
44 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| // Copyright (c) 2019, Daniel Martí <mvdan@mvdan.cc> | ||
| // See LICENSE for licensing information | ||
|
|
||
| // TODO: replace with the unix build tag once we require Go 1.19 or later | ||
| //go:build linux | ||
|
|
||
| package main | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "fmt" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "strconv" | ||
| "testing" | ||
|
|
||
| qt "github.com/frankban/quicktest" | ||
| "golang.org/x/sys/unix" | ||
| ) | ||
|
|
||
| func init() { | ||
| // Here rather than in TestMain, to reuse the unix build tag. | ||
| if limit := os.Getenv("TEST_WITH_FILE_LIMIT"); limit != "" { | ||
| n, err := strconv.ParseUint(limit, 10, 64) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| rlimit := unix.Rlimit{Cur: n, Max: n} | ||
| if err := unix.Setrlimit(unix.RLIMIT_NOFILE, &rlimit); err != nil { | ||
| panic(err) | ||
| } | ||
| os.Exit(main1()) | ||
| } | ||
| } | ||
|
|
||
| func TestWithLowOpenFileLimit(t *testing.T) { | ||
| // Safe to run in parallel, as we only change the limit for child processes. | ||
| t.Parallel() | ||
|
|
||
| tempDir := t.TempDir() | ||
| testBinary, err := os.Executable() | ||
| qt.Assert(t, err, qt.IsNil) | ||
|
|
||
| const ( | ||
| // Enough directories to run into the ulimit. | ||
| // Enough number of files in total to run into the ulimit. | ||
| numberDirs = 500 | ||
| numberFilesPerDir = 20 | ||
| numberFilesTotal = numberDirs * numberFilesPerDir | ||
| ) | ||
| t.Logf("writing %d tiny Go files", numberFilesTotal) | ||
| var allGoFiles []string | ||
| for i := 0; i < numberDirs; i++ { | ||
| // Prefix "p", so the package name is a valid identifier. | ||
| // Add one go.mod file per directory as well, | ||
| // which will help catch data races when loading module info. | ||
| dirName := fmt.Sprintf("p%03d", i) | ||
| dirPath := filepath.Join(tempDir, dirName) | ||
| err := os.MkdirAll(dirPath, 0o777) | ||
| qt.Assert(t, err, qt.IsNil) | ||
|
|
||
| err = os.WriteFile(filepath.Join(dirPath, "go.mod"), | ||
| []byte(fmt.Sprintf("module %s\n\ngo 1.16", dirName)), 0o666) | ||
| qt.Assert(t, err, qt.IsNil) | ||
|
|
||
| for j := 0; j < numberFilesPerDir; j++ { | ||
| filePath := filepath.Join(dirPath, fmt.Sprintf("%03d.go", j)) | ||
| err := os.WriteFile(filePath, | ||
| // Extra newlines so that "-l" prints all paths. | ||
| []byte(fmt.Sprintf("package %s\n\n\n", dirName)), 0o666) | ||
| qt.Assert(t, err, qt.IsNil) | ||
| allGoFiles = append(allGoFiles, filePath) | ||
| } | ||
| } | ||
| if len(allGoFiles) != numberFilesTotal { | ||
| panic("allGoFiles doesn't have the expected number of files?") | ||
| } | ||
| runGofmt := func(paths ...string) { | ||
| t.Logf("running with %d paths", len(paths)) | ||
| cmd := exec.Command(testBinary, append([]string{"-l"}, paths...)...) | ||
| // 256 is a relatively common low limit, e.g. on Mac. | ||
| cmd.Env = append(os.Environ(), "TEST_WITH_FILE_LIMIT=256") | ||
| out, err := cmd.Output() | ||
| var stderr []byte | ||
| if err, _ := err.(*exec.ExitError); err != nil { | ||
| stderr = err.Stderr | ||
| } | ||
| qt.Assert(t, err, qt.IsNil, qt.Commentf("stderr:\n%s", stderr)) | ||
| qt.Assert(t, bytes.Count(out, []byte("\n")), qt.Equals, len(allGoFiles)) | ||
| } | ||
| runGofmt(tempDir) | ||
| runGofmt(allGoFiles...) | ||
| } |