cmd/go/internal/modfetch: lock files and directories

We employ the following new locking mechanisms:

• Zip files and list files within the module cache are written using
  atomic renames of temporary files, so that GOPROXY servers reading
  from the cache will never serve incomplete content.

• A lock file for each module version guards downloading and extraction of
  (immutable) module contents.

• A lock file alongside each version list (named 'list.lock')
  guards updates to the list.

• A single lock file in the module cache guards updates to all go.sum
  files. The go.sum files themselves are written using an atomic
  rename to ensure that we never accidentally discard existing sums.

Updates #26794

RELNOTE=yes

Change-Id: I16ef8b06ee4bd7b94d0c0a6f5d17e1cecc379076
Reviewed-on: https://go-review.googlesource.com/c/146382
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>

src/cmd/go/internal/clean/clean.go

@@ -176,27 +176,13 @@ func runClean(cmd *base.Command, args []string) {
 			b.Showcmd("", "rm -rf %s", modfetch.PkgMod)
 		}
 		if !cfg.BuildN {
-			if err := removeAll(modfetch.PkgMod); err != nil {
+			if err := modfetch.RemoveAll(modfetch.PkgMod); err != nil {
 				base.Errorf("go clean -modcache: %v", err)
 			}
 		}
 	}
 }
 
-func removeAll(dir string) error {
-	// Module cache has 0555 directories; make them writable in order to remove content.
-	filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
-		if err != nil {
-			return nil // ignore errors walking in file system
-		}
-		if info.IsDir() {
-			os.Chmod(path, 0777)
-		}
-		return nil
-	})
-	return os.RemoveAll(dir)
-}
-
 var cleaned = map[*load.Package]bool{}
 
 // TODO: These are dregs left by Makefile-based builds.

src/cmd/go/internal/modfetch/cache.go

@@ -15,9 +15,11 @@ import (
 	"strings"
 
 	"cmd/go/internal/base"
+	"cmd/go/internal/lockedfile"
 	"cmd/go/internal/modfetch/codehost"
 	"cmd/go/internal/module"
 	"cmd/go/internal/par"
+	"cmd/go/internal/renameio"
 	"cmd/go/internal/semver"
 )
 
@@ -75,6 +77,37 @@ func DownloadDir(m module.Version) (string, error) {
 	return filepath.Join(PkgMod, enc+"@"+encVer), nil
 }
 
+// lockVersion locks a file within the module cache that guards the downloading
+// and extraction of the zipfile for the given module version.
+func lockVersion(mod module.Version) (unlock func(), err error) {
+	path, err := CachePath(mod, "lock")
+	if err != nil {
+		return nil, err
+	}
+	if err := os.MkdirAll(filepath.Dir(path), 0777); err != nil {
+		return nil, err
+	}
+	return lockedfile.MutexAt(path).Lock()
+}
+
+// SideLock locks a file within the module cache that that guards edits to files
+// outside the cache, such as go.sum and go.mod files in the user's working
+// directory. It returns a function that must be called to unlock the file.
+func SideLock() (unlock func()) {
+	if PkgMod == "" {
+		base.Fatalf("go: internal error: modfetch.PkgMod not set")
+	}
+	path := filepath.Join(PkgMod, "cache", "lock")
+	if err := os.MkdirAll(filepath.Dir(path), 0777); err != nil {
+		base.Fatalf("go: failed to create cache directory %s: %v", filepath.Dir(path), err)
+	}
+	unlock, err := lockedfile.MutexAt(path).Lock()
+	if err != nil {
+		base.Fatalf("go: failed to lock file at %v", path)
+	}
+	return unlock
+}
+
 // A cachingRepo is a cache around an underlying Repo,
 // avoiding redundant calls to ModulePath, Versions, Stat, Latest, and GoMod (but not Zip).
 // It is also safe for simultaneous use by multiple goroutines
@@ -386,7 +419,7 @@ func readDiskStatByHash(path, rev string) (file string, info *RevInfo, err error
 // and should ignore it.
 var oldVgoPrefix = []byte("//vgo 0.0.")
 
-// readDiskGoMod reads a cached stat result from disk,
+// readDiskGoMod reads a cached go.mod file from disk,
 // returning the name of the cache file and the result.
 // If the read fails, the caller can use
 // writeDiskGoMod(file, data) to write a new cache entry.
@@ -452,22 +485,8 @@ func writeDiskCache(file string, data []byte) error {
 	if err := os.MkdirAll(filepath.Dir(file), 0777); err != nil {
 		return err
 	}
-	// Write data to temp file next to target file.
-	f, err := ioutil.TempFile(filepath.Dir(file), filepath.Base(file)+".tmp-")
-	if err != nil {
-		return err
-	}
-	defer os.Remove(f.Name())
-	defer f.Close()
-	if _, err := f.Write(data); err != nil {
-		return err
-	}
-	if err := f.Close(); err != nil {
-		return err
-	}
-	// Rename temp file onto cache file,
-	// so that the cache file is always a complete file.
-	if err := os.Rename(f.Name(), file); err != nil {
+
+	if err := renameio.WriteFile(file, data); err != nil {
 		return err
 	}
 
@@ -484,8 +503,18 @@ func rewriteVersionList(dir string) {
 		base.Fatalf("go: internal error: misuse of rewriteVersionList")
 	}
 
-	// TODO(rsc): We should do some kind of directory locking here,
-	// to avoid lost updates.
+	listFile := filepath.Join(dir, "list")
+
+	// We use a separate lockfile here instead of locking listFile itself because
+	// we want to use Rename to write the file atomically. The list may be read by
+	// a GOPROXY HTTP server, and if we crash midway through a rewrite (or if the
+	// HTTP server ignores our locking and serves the file midway through a
+	// rewrite) it's better to serve a stale list than a truncated one.
+	unlock, err := lockedfile.MutexAt(listFile + ".lock").Lock()
+	if err != nil {
+		base.Fatalf("go: can't lock version list lockfile: %v", err)
+	}
+	defer unlock()
 
 	infos, err := ioutil.ReadDir(dir)
 	if err != nil {
@@ -514,12 +543,12 @@ func rewriteVersionList(dir string) {
 		buf.WriteString(v)
 		buf.WriteString("\n")
 	}
-	listFile := filepath.Join(dir, "list")
 	old, _ := ioutil.ReadFile(listFile)
 	if bytes.Equal(buf.Bytes(), old) {
 		return
 	}
-	// TODO: Use rename to install file,
-	// so that readers never see an incomplete file.
-	ioutil.WriteFile(listFile, buf.Bytes(), 0666)
+
+	if err := renameio.WriteFile(listFile, buf.Bytes()); err != nil {
+		base.Fatalf("go: failed to write version list: %v", err)
+	}
 }

src/cmd/go/internal/modfetch/unzip.go

@@ -21,12 +21,12 @@ import (
 )
 
 func Unzip(dir, zipfile, prefix string, maxSize int64) error {
+	// TODO(bcmills): The maxSize parameter is invariantly 0. Remove it.
 	if maxSize == 0 {
 		maxSize = codehost.MaxZipFile
 	}
 
 	// Directory can exist, but must be empty.
-	// except maybe
 	files, _ := ioutil.ReadDir(dir)
 	if len(files) > 0 {
 		return fmt.Errorf("target directory %v exists and is not empty", dir)
@@ -113,7 +113,7 @@ func Unzip(dir, zipfile, prefix string, maxSize int64) error {
 		if err := os.MkdirAll(filepath.Dir(dst), 0777); err != nil {
 			return err
 		}
-		w, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0444)
+		w, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0444)
 		if err != nil {
 			return fmt.Errorf("unzip %v: %v", zipfile, err)
 		}
@@ -143,11 +143,27 @@ func Unzip(dir, zipfile, prefix string, maxSize int64) error {
 		dirlist = append(dirlist, dir)
 	}
 	sort.Strings(dirlist)
-
 	// Run over list backward to chmod children before parents.
 	for i := len(dirlist) - 1; i >= 0; i-- {
+		// TODO(bcmills): Does this end up stomping on the umask of the cache directory?
 		os.Chmod(dirlist[i], 0555)
 	}
 
 	return nil
 }
+
+// RemoveAll removes a directory written by Download or Unzip, first applying
+// any permission changes needed to do so.
+func RemoveAll(dir string) error {
+	// Module cache has 0555 directories; make them writable in order to remove content.
+	filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return nil // ignore errors walking in file system
+		}
+		if info.IsDir() {
+			os.Chmod(path, 0777)
+		}
+		return nil
+	})
+	return os.RemoveAll(dir)
+}

src/cmd/go/testdata/script/mod_concurrent.txt

+env GO111MODULE=on
+
+# Concurrent builds should succeed, even if they need to download modules.
+go build ./x &
+go build ./y
+wait
+
+# Concurrent builds should update go.sum to the union of the hashes for the
+# modules they read.
+cmp go.sum go.sum.want
+
+-- go.mod --
+module golang.org/issue/26794
+
+require (
+	golang.org/x/text v0.3.0
+	rsc.io/sampler v1.0.0
+)
+-- x/x.go --
+package x
+
+import _ "golang.org/x/text/language"
+-- y/y.go --
+package y
+
+import _ "rsc.io/sampler"
+-- go.sum.want --
+golang.org/x/text v0.3.0 h1:ivTorhoiROmZ1mcs15mO2czVF0uy0tnezXpBVNzgrmA=
+golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
+rsc.io/sampler v1.0.0 h1:SRJnjyQ07sAtq6G4RcfJEmz8JxqLyj3PoGXG2VhbDWo=
+rsc.io/sampler v1.0.0/go.mod h1:cqxpM3ZVz9VtirqxZPmrWzkQ+UkiNiGtkrN+B+i8kx8=