From: Jay Conrod Date: Tue, 25 Feb 2020 16:00:08 +0000 (-0500) Subject: cmd/go: extract module zip files in place X-Git-Tag: go1.15beta1~892 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=576fa692774137633b09dd244e1de36993dd2803;p=gostls13.git cmd/go: extract module zip files in place Previously, we extracted module zip files to temporary directories with random names, then renamed them to their final locations. This failed with ERROR_ACCESS_DENIED on Windows if any file in the temporary was open. Antivirus programs did this occasionally. Retrying the rename did not work (CL 220978). With this change, we extract module zip files in place. We create a .partial file alongside the .lock file to indicate a directory is not fully populated, and we delete this at the end of the process. Updates #36568 Change-Id: I75c09df879a602841f3459322c021896292b2fdb Reviewed-on: https://go-review.googlesource.com/c/go/+/221157 Run-TryBot: Jay Conrod TryBot-Result: Gobot Gobot Reviewed-by: Bryan C. Mills Reviewed-by: Michael Matloob --- diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go index 5787e14aa0..187d174542 100644 --- a/src/cmd/go/internal/modfetch/fetch.go +++ b/src/cmd/go/internal/modfetch/fetch.go @@ -57,11 +57,10 @@ func Download(mod module.Version) (dir string, err error) { } func download(mod module.Version) (dir string, err error) { - // If the directory exists, and no .partial file exists, - // the module has already been completely extracted. - // .partial files may be created when future versions of cmd/go - // extract module zip directories in place instead of extracting - // to a random temporary directory and renaming. + // If the directory exists, and no .partial file exists, the module has + // already been completely extracted. .partial files may be created when a + // module zip directory is extracted in place instead of being extracted to a + // temporary directory and renamed. dir, err = DownloadDir(mod) if err == nil { return dir, nil @@ -115,34 +114,61 @@ func download(mod module.Version) (dir string, err error) { return "", err } - // Extract the zip file to a temporary directory, then rename it to the - // final path. That way, we can use the existence of the source directory to - // signal that it has been extracted successfully, and if someone deletes - // the entire directory (e.g. as an attempt to prune out file corruption) - // the module cache will still be left in a recoverable state. - // We retry failed renames using robustio.Rename on Windows. Programs that - // open files in the temporary directory (antivirus, search indexers, etc.) - // can cause os.Rename to fail with ERROR_ACCESS_DENIED. + // Extract the module zip directory. + // + // By default, we extract to a temporary directory, then atomically rename to + // its final location. We use the existence of the source directory to signal + // that it has been extracted successfully (see DownloadDir). If someone + // deletes the entire directory (e.g., as an attempt to prune out file + // corruption), the module cache will still be left in a recoverable + // state. + // + // Unfortunately, os.Rename may fail with ERROR_ACCESS_DENIED on Windows if + // another process opens files in the temporary directory. This is partially + // mitigated by using robustio.Rename, which retries os.Rename for a short + // time. + // + // To avoid this error completely, if unzipInPlace is set, we instead create a + // .partial file (indicating the directory isn't fully extracted), then we + // extract the directory at its final location, then we delete the .partial + // file. This is not the default behavior because older versions of Go may + // simply stat the directory to check whether it exists without looking for a + // .partial file. If multiple versions run concurrently, the older version may + // assume a partially extracted directory is complete. + // TODO(golang.org/issue/36568): when these older versions are no longer + // supported, remove the old default behavior and the unzipInPlace flag. if err := os.MkdirAll(parentDir, 0777); err != nil { return "", err } - tmpDir, err := ioutil.TempDir(parentDir, tmpPrefix) - if err != nil { - return "", err - } - defer func() { + + if unzipInPlace { + if err := ioutil.WriteFile(partialPath, nil, 0666); err != nil { + return "", err + } + if err := modzip.Unzip(dir, mod, zipfile); err != nil { + fmt.Fprintf(os.Stderr, "-> %s\n", err) + if rmErr := RemoveAll(dir); rmErr == nil { + os.Remove(partialPath) + } + return "", err + } + if err := os.Remove(partialPath); err != nil { + return "", err + } + } else { + tmpDir, err := ioutil.TempDir(parentDir, tmpPrefix) if err != nil { + return "", err + } + if err := modzip.Unzip(tmpDir, mod, zipfile); err != nil { + fmt.Fprintf(os.Stderr, "-> %s\n", err) RemoveAll(tmpDir) + return "", err + } + if err := robustio.Rename(tmpDir, dir); err != nil { + RemoveAll(tmpDir) + return "", err } - }() - - if err := modzip.Unzip(tmpDir, mod, zipfile); err != nil { - fmt.Fprintf(os.Stderr, "-> %s\n", err) - return "", err - } - - if err := robustio.Rename(tmpDir, dir); err != nil { - return "", err } if !cfg.ModCacheRW { @@ -153,6 +179,17 @@ func download(mod module.Version) (dir string, err error) { return dir, nil } +var unzipInPlace bool + +func init() { + for _, f := range strings.Split(os.Getenv("GODEBUG"), ",") { + if f == "modcacheunzipinplace=1" { + unzipInPlace = true + break + } + } +} + var downloadZipCache par.Cache // DownloadZip downloads the specific module version to the @@ -324,7 +361,7 @@ func RemoveAll(dir string) error { } return nil }) - return os.RemoveAll(dir) + return robustio.RemoveAll(dir) } var GoSumFile string // path to go.sum; set by package modload diff --git a/src/cmd/go/script_test.go b/src/cmd/go/script_test.go index 1dca486c91..00c6523cbc 100644 --- a/src/cmd/go/script_test.go +++ b/src/cmd/go/script_test.go @@ -109,6 +109,7 @@ func (ts *testScript) setup() { "CCACHE_DISABLE=1", // ccache breaks with non-existent HOME "GOARCH=" + runtime.GOARCH, "GOCACHE=" + testGOCACHE, + "GODEBUG=" + os.Getenv("GODEBUG"), "GOEXE=" + cfg.ExeSuffix, "GOOS=" + runtime.GOOS, "GOPATH=" + filepath.Join(ts.workdir, "gopath"), diff --git a/src/cmd/go/testdata/script/README b/src/cmd/go/testdata/script/README index 71d38161d5..65b4c78090 100644 --- a/src/cmd/go/testdata/script/README +++ b/src/cmd/go/testdata/script/README @@ -36,6 +36,7 @@ Scripts also have access to these other environment variables: HOME=/no-home PATH= TMPDIR=$WORK/tmp + GODEBUG= devnull= goversion= := diff --git a/src/cmd/go/testdata/script/mod_concurrent_unzipinplace.txt b/src/cmd/go/testdata/script/mod_concurrent_unzipinplace.txt new file mode 100644 index 0000000000..473be71c9c --- /dev/null +++ b/src/cmd/go/testdata/script/mod_concurrent_unzipinplace.txt @@ -0,0 +1,17 @@ +# This tests checks the GODEBUG=modcacheunzipinplace=1 flag, used as part of +# a migration in golang.org/issue/36568. +# +# Concurrent downloads with and without GODEBUG=modcacheunzipinplace=1 should +# not conflict. This is meant to simulate an old version and a new version +# of Go accessing the cache concurrently. +go mod download & +env GODEBUG=modcacheunzipinplace=1 +go mod download +wait + +-- go.mod -- +module golang.org/issue/36568 + +go 1.14 + +require rsc.io/quote v1.5.2 diff --git a/src/cmd/go/testdata/script/mod_download_concurrent_read.txt b/src/cmd/go/testdata/script/mod_download_concurrent_read.txt new file mode 100644 index 0000000000..bb9c588896 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_download_concurrent_read.txt @@ -0,0 +1,120 @@ +# This test simulates a process watching for changes and reading files in +# module cache as a module is extracted. +# +# By default, we unzip a downloaded module into a temporary directory with a +# random name, then rename the directory into place. On Windows, this fails +# with ERROR_ACCESS_DENIED if another process (e.g., antivirus) opens files +# in the directory. +# +# Setting GODEBUG=modcacheunzipinplace=1 opts into new behavior: a downloaded +# module is unzipped in place. A .partial file is created elsewhere to indicate +# that the extraction is incomplete. +# +# Verifies golang.org/issue/36568. + +[!windows] skip +[short] skip + +# Control case: check that the default behavior fails. +# This is commented out to avoid flakiness. We can't reproduce the failure +# 100% of the time. +# ! go run downloader.go + +# Experiment: check that the new behavior does not fail. +env GODEBUG=modcacheunzipinplace=1 +go run downloader.go + +-- go.mod -- +module example.com/m + +go 1.14 + +-- downloader.go -- +package main + +import ( + "fmt" + "io/ioutil" + "log" + "os" + "os/exec" + "path/filepath" +) + +func main() { + if err := run(); err != nil { + log.Fatal(err) + } +} + +// run repeatedly downloads a module while opening files in the module cache +// in a background goroutine. +// +// run uses a different temporary module cache in each iteration so that we +// don't need to clean the cache or synchronize closing files after each +// iteration. +func run() (err error) { + tmpDir, err := ioutil.TempDir("", "") + if err != nil { + return err + } + defer func() { + if rmErr := os.RemoveAll(tmpDir); err == nil && rmErr != nil { + err = rmErr + } + }() + for i := 0; i < 10; i++ { + gopath := filepath.Join(tmpDir, fmt.Sprintf("gopath%d", i)) + var err error + done := make(chan struct{}) + go func() { + err = download(gopath) + close(done) + }() + readCache(gopath, done) + if err != nil { + return err + } + } + return nil +} + +// download downloads a module into the given cache using 'go mod download'. +func download(gopath string) error { + cmd := exec.Command("go", "mod", "download", "-modcacherw", "rsc.io/quote@v1.5.2") + cmd.Stderr = os.Stderr + cmd.Env = append(os.Environ(), "GOPATH="+gopath) + return cmd.Run() +} + +// readCache repeatedly globs for go.mod files in the given cache, then opens +// those files for reading. When the done chan is closed, readCache closes +// files and returns. +func readCache(gopath string, done <-chan struct{}) { + files := make(map[string]*os.File) + defer func() { + for _, f := range files { + f.Close() + } + }() + + pattern := filepath.Join(gopath, "pkg/mod/rsc.io/quote@v1.5.2*/go.mod") + for { + select { + case <-done: + return + default: + } + + names, _ := filepath.Glob(pattern) + for _, name := range names { + if files[name] != nil { + continue + } + f, _ := os.Open(name) + if f != nil { + files[name] = f + } + } + } +} diff --git a/src/cmd/go/testdata/script/mod_download_partial.txt b/src/cmd/go/testdata/script/mod_download_partial.txt index b035382296..4978982dab 100644 --- a/src/cmd/go/testdata/script/mod_download_partial.txt +++ b/src/cmd/go/testdata/script/mod_download_partial.txt @@ -44,6 +44,16 @@ go mod verify rm $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/go.mod ! go mod verify +# 'go mod download' should not leave behind a directory or a .partial file +# if there is an error extracting the zip file. +env GODEBUG=modcacheunzipinplace=1 +rm $GOPATH/pkg/mod/rsc.io/quote@v1.5.2 +cp empty $GOPATH/pkg/mod/cache/download/rsc.io/quote/@v/v1.5.2.zip +! go mod download +stderr 'not a valid zip file' +! exists $GOPATH/pkg/mod/rsc.io/quote@v1.5.2 +! exists $GOPATH/pkg/mod/cache/download/rsc.io/quote/@v/v1.5.2.partial + -- go.mod -- module m