]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go/internal/work: make NewBuilder safe for concurrent and repeated use
authorBryan C. Mills <bcmills@google.com>
Tue, 23 Aug 2022 20:33:01 +0000 (16:33 -0400)
committerGopher Robot <gobot@golang.org>
Wed, 24 Aug 2022 16:06:13 +0000 (16:06 +0000)
Ever since 'go build' was added (in CL 5483069), it has used an atexit
handler to clean up working directories. At some point (prior to CL
95900044), Init was called multiple times per builder, registering
potentially many atexit handlers that execute asynchronously and make
debugging more difficult.

The use of an AtExit handler also makes the Builder (and anything that
uses it) prone to races: the base.AtExit API is not designed for
concurrent use, but cmd/go is becoming increasingly concurrent over
time. The AtExit handler also makes the Builder inappropriate to use
within a unit-test, since the handlers do not run during the test
function and accumulate over time.

This change makes NewBuilder safe for concurrent use by registering
the AtExit handler only once (during BuildInit, which was already not
safe for concurrent use), and using a sync.Map to store the set of
builders that need cleanup in case of an unclean exit. In addition, it
causes the test variant of cmd/go to fail if any Builder instance
leaks from a clean exit, helping to ensure that functions that create
Builders do not leak them indefinitely, especially in tests.

Updates #54423.

Change-Id: Ia227b15b8fa53c33177c71271d756ac0858feebe
Reviewed-on: https://go-review.googlesource.com/c/go/+/425254
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>

src/cmd/go/internal/bug/bug.go
src/cmd/go/internal/envcmd/env.go
src/cmd/go/internal/list/list.go
src/cmd/go/internal/run/run.go
src/cmd/go/internal/test/test.go
src/cmd/go/internal/vet/vet.go
src/cmd/go/internal/work/action.go
src/cmd/go/internal/work/build.go
src/cmd/go/internal/work/init.go
src/cmd/go/script_test.go

index 9c9e9dd68adbd009d3be4a7f3b641015dc20a317..f1c6b413286bdbd79b011acf32f293991cc160ea 100644 (file)
@@ -22,6 +22,7 @@ import (
        "cmd/go/internal/cfg"
        "cmd/go/internal/envcmd"
        "cmd/go/internal/web"
+       "cmd/go/internal/work"
 )
 
 var CmdBug = &base.Command{
@@ -42,6 +43,8 @@ func runBug(ctx context.Context, cmd *base.Command, args []string) {
        if len(args) > 0 {
                base.Fatalf("go: bug takes no arguments")
        }
+       work.BuildInit()
+
        var buf bytes.Buffer
        buf.WriteString(bugHeader)
        printGoVersion(&buf)
index bbd3318d26b8905b253ba6b22622d5c07b062125..6dd8657bfc61b360d9e83f916ba6afde141b6845 100644 (file)
@@ -175,6 +175,11 @@ func ExtraEnvVars() []cfg.EnvVar {
 // but are costly to evaluate.
 func ExtraEnvVarsCostly() []cfg.EnvVar {
        b := work.NewBuilder("")
+       defer func() {
+               if err := b.Close(); err != nil {
+                       base.Fatalf("go: %v", err)
+               }
+       }()
 
        cppflags, cflags, cxxflags, fflags, ldflags, err := b.CFlags(&load.Package{})
        if err != nil {
@@ -272,6 +277,7 @@ func runEnv(ctx context.Context, cmd *base.Command, args []string) {
                }
        }
        if needCostly {
+               work.BuildInit()
                env = append(env, ExtraEnvVarsCostly()...)
        }
 
index 0b928d27e6c95e71eabce41390cdf2d1e932d9ec..66c33d9adefae8c93dfc727e60d58c283a504743 100644 (file)
@@ -690,6 +690,12 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
        needStale := (listJson && listJsonFields.needAny("Stale", "StaleReason")) || strings.Contains(*listFmt, ".Stale")
        if needStale || *listExport || *listCompiled {
                b := work.NewBuilder("")
+               defer func() {
+                       if err := b.Close(); err != nil {
+                               base.Fatalf("go: %v", err)
+                       }
+               }()
+
                b.IsCmdList = true
                b.NeedExport = *listExport
                b.NeedCompiledGoFiles = *listCompiled
index 6b253a2c9e52e5caff3bfbb197371ff97cf8d695..2804db2296419dfdafbf08bac25fc5632a4deb6f 100644 (file)
@@ -92,6 +92,11 @@ func runRun(ctx context.Context, cmd *base.Command, args []string) {
 
        work.BuildInit()
        b := work.NewBuilder("")
+       defer func() {
+               if err := b.Close(); err != nil {
+                       base.Fatalf("go: %v", err)
+               }
+       }()
        b.Print = printStderr
 
        i := 0
index 028db84cc462bc7a72683c741c7cde0407a1e4cd..7e6747055e04acfbc4407dd237bc0c01f6813e58 100644 (file)
@@ -745,6 +745,11 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
        }
 
        b := work.NewBuilder("")
+       defer func() {
+               if err := b.Close(); err != nil {
+                       base.Fatalf("go: %v", err)
+               }
+       }()
 
        if cfg.BuildI {
                fmt.Fprint(os.Stderr, "go: -i flag is deprecated\n")
@@ -808,6 +813,9 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
                //
                // Maybe this has the effect of removing actions that were registered by the
                // call to CompileAction above?
+               if err := b.Close(); err != nil {
+                       base.Fatalf("go: %v", err)
+               }
                b = work.NewBuilder("")
        }
 
index 085c7d23b246998adf1264985aed0ac0a5b8b4dd..ee672d1a30d8a058f147a66227b8ec4076be9dd2 100644 (file)
@@ -95,6 +95,11 @@ func runVet(ctx context.Context, cmd *base.Command, args []string) {
        }
 
        b := work.NewBuilder("")
+       defer func() {
+               if err := b.Close(); err != nil {
+                       base.Fatalf("go: %v", err)
+               }
+       }()
 
        root := &work.Action{Mode: "go vet"}
        for _, p := range pkgs {
index 4bbd23ab8e43212b10a1303cfec594a16afff59e..ae9afd2f123fb71b8b940b62180fc9d56e5cfe73 100644 (file)
@@ -16,7 +16,6 @@ import (
        "fmt"
        "os"
        "path/filepath"
-       "runtime"
        "strings"
        "sync"
        "time"
@@ -25,6 +24,7 @@ import (
        "cmd/go/internal/cache"
        "cmd/go/internal/cfg"
        "cmd/go/internal/load"
+       "cmd/go/internal/robustio"
        "cmd/go/internal/trace"
        "cmd/internal/buildid"
 )
@@ -244,6 +244,8 @@ const (
 //
 // If workDir is the empty string, NewBuilder creates a WorkDir if needed
 // and arranges for it to be removed in case of an unclean exit.
+// The caller must Close the builder explicitly to clean up the WorkDir
+// before a clean exit.
 func NewBuilder(workDir string) *Builder {
        b := new(Builder)
 
@@ -260,6 +262,9 @@ func NewBuilder(workDir string) *Builder {
        } else if cfg.BuildN {
                b.WorkDir = "$WORK"
        } else {
+               if !buildInitStarted {
+                       panic("internal error: NewBuilder called before BuildInit")
+               }
                tmp, err := os.MkdirTemp(cfg.Getenv("GOTMPDIR"), "go-build")
                if err != nil {
                        base.Fatalf("go: creating work dir: %v", err)
@@ -273,32 +278,10 @@ func NewBuilder(workDir string) *Builder {
                        tmp = abs
                }
                b.WorkDir = tmp
+               builderWorkDirs.Store(b, b.WorkDir)
                if cfg.BuildX || cfg.BuildWork {
                        fmt.Fprintf(os.Stderr, "WORK=%s\n", b.WorkDir)
                }
-               if !cfg.BuildWork {
-                       workdir := b.WorkDir
-                       base.AtExit(func() {
-                               start := time.Now()
-                               for {
-                                       err := os.RemoveAll(workdir)
-                                       if err == nil {
-                                               return
-                                       }
-
-                                       // On some configurations of Windows, directories containing executable
-                                       // files may be locked for a while after the executable exits (perhaps
-                                       // due to antivirus scans?). It's probably worth a little extra latency
-                                       // on exit to avoid filling up the user's temporary directory with leaked
-                                       // files. (See golang.org/issue/30789.)
-                                       if runtime.GOOS != "windows" || time.Since(start) >= 500*time.Millisecond {
-                                               fmt.Fprintf(os.Stderr, "go: failed to remove work dir: %s\n", err)
-                                               return
-                                       }
-                                       time.Sleep(5 * time.Millisecond)
-                               }
-                       })
-               }
        }
 
        if err := CheckGOOSARCHPair(cfg.Goos, cfg.Goarch); err != nil {
@@ -318,6 +301,44 @@ func NewBuilder(workDir string) *Builder {
        return b
 }
 
+var builderWorkDirs sync.Map // *Builder → WorkDir
+
+func (b *Builder) Close() error {
+       wd, ok := builderWorkDirs.Load(b)
+       if !ok {
+               return nil
+       }
+       defer builderWorkDirs.Delete(b)
+
+       if b.WorkDir != wd.(string) {
+               base.Errorf("go: internal error: Builder WorkDir unexpectedly changed from %s to %s", wd, b.WorkDir)
+       }
+
+       if !cfg.BuildWork {
+               if err := robustio.RemoveAll(b.WorkDir); err != nil {
+                       return err
+               }
+       }
+       b.WorkDir = ""
+       return nil
+}
+
+func closeBuilders() {
+       leakedBuilders := 0
+       builderWorkDirs.Range(func(bi, _ any) bool {
+               leakedBuilders++
+               if err := bi.(*Builder).Close(); err != nil {
+                       base.Errorf("go: %v", err)
+               }
+               return true
+       })
+
+       if leakedBuilders > 0 && base.GetExitStatus() == 0 {
+               fmt.Fprintf(os.Stderr, "go: internal error: Builder leaked on successful exit\n")
+               base.SetExitStatus(1)
+       }
+}
+
 func CheckGOOSARCHPair(goos, goarch string) error {
        if _, ok := cfg.OSArchSupportsCgo[goos+"/"+goarch]; !ok && cfg.BuildContext.Compiler == "gc" {
                return fmt.Errorf("unsupported GOOS/GOARCH pair %s/%s", goos, goarch)
index e34cacca039a47a72c1f5c9e116163b4b7b968f2..bce923a459551136a68b2d9356f613cef58b57b6 100644 (file)
@@ -404,6 +404,11 @@ func runBuild(ctx context.Context, cmd *base.Command, args []string) {
        modload.InitWorkfile()
        BuildInit()
        b := NewBuilder("")
+       defer func() {
+               if err := b.Close(); err != nil {
+                       base.Fatalf("go: %v", err)
+               }
+       }()
 
        pkgs := load.PackagesAndErrors(ctx, load.PackageOpts{AutoVCS: true}, args)
        load.CheckPackageErrors(pkgs)
@@ -728,6 +733,11 @@ func InstallPackages(ctx context.Context, patterns []string, pkgs []*load.Packag
        base.ExitIfErrors()
 
        b := NewBuilder("")
+       defer func() {
+               if err := b.Close(); err != nil {
+                       base.Fatalf("go: %v", err)
+               }
+       }()
 
        depMode := ModeBuild
        if cfg.BuildI {
index 255ff3a0c50d84c0016a63929e8385c3f4260694..67bd6a4c6705704075f3a05776ce10bf71151f34 100644 (file)
@@ -24,7 +24,15 @@ import (
        "sync"
 )
 
+var buildInitStarted = false
+
 func BuildInit() {
+       if buildInitStarted {
+               base.Fatalf("go: internal error: work.BuildInit called more than once")
+       }
+       buildInitStarted = true
+       base.AtExit(closeBuilders)
+
        modload.Init()
        instrumentInit()
        buildModeInit()
index ca76ab5ab8e387e3b380792415331f71bb3d3df1..b2f68b67f9eb2bbde1ab23cf5caed95d15987c09 100644 (file)
@@ -577,6 +577,11 @@ func (ts *testScript) cmdCc(want simpleStatus, args []string) {
        }
 
        b := work.NewBuilder(ts.workdir)
+       defer func() {
+               if err := b.Close(); err != nil {
+                       ts.fatalf("%v", err)
+               }
+       }()
        ts.cmdExec(want, append(b.GccCmd(".", ""), args...))
 }