]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go/internal/modfetch: always extract module directories in place
authorJay Conrod <jayconrod@google.com>
Thu, 1 Oct 2020 17:37:06 +0000 (13:37 -0400)
committerJay Conrod <jayconrod@google.com>
Thu, 1 Oct 2020 21:06:35 +0000 (21:06 +0000)
Previously by default, we extracted modules to a temporary directory,
then renamed it into place. This failed with ERROR_ACCESS_DENIED on
Windows if another process (usually an anti-virus scanner) opened
files in the temporary directory.

Since Go 1.15, users have been able to set
GODEBUG=modcacheunzipinplace=1 to opt into new behavior: we extract
modules at their final location, and we create and later delete a
.partial file to prevent the directory from being used if we crash.
.partial files are recognized by Go 1.14.2 and later.

With this change, the new behavior is the only behavior.
modcacheunzipinplace is no longer recognized.

Fixes #36568

Change-Id: Iff19fca5cd6eaa3597975a69fa05c4cb1b834bd6
Reviewed-on: https://go-review.googlesource.com/c/go/+/258798
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
src/cmd/go/internal/modfetch/fetch.go
src/cmd/go/testdata/script/mod_concurrent_unzipinplace.txt [deleted file]
src/cmd/go/testdata/script/mod_download_concurrent_read.txt
src/cmd/go/testdata/script/mod_download_partial.txt

index 01d8f007ac6ce2e5fd60f4a0a08e3421b9431bdc..1d90002faad08aa0aa2aedccac7b09677b06a45e 100644 (file)
@@ -63,12 +63,9 @@ func download(ctx context.Context, mod module.Version) (dir string, err error) {
        ctx, span := trace.StartSpan(ctx, "modfetch.download "+mod.String())
        defer span.Done()
 
-       // 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 {
+               // The directory has already been completely extracted (no .partial file exists).
                return dir, nil
        } else if dir == "" || !errors.Is(err, os.ErrNotExist) {
                return "", err
@@ -88,6 +85,9 @@ func download(ctx context.Context, mod module.Version) (dir string, err error) {
        }
        defer unlock()
 
+       ctx, span = trace.StartSpan(ctx, "unzip "+zipfile)
+       defer span.Done()
+
        // Check whether the directory was populated while we were waiting on the lock.
        _, dirErr := DownloadDir(mod)
        if dirErr == nil {
@@ -95,10 +95,11 @@ func download(ctx context.Context, mod module.Version) (dir string, err error) {
        }
        _, dirExists := dirErr.(*DownloadDirPartialError)
 
-       // Clean up any remaining temporary directories from previous runs, as well
-       // as partially extracted diectories created by future versions of cmd/go.
-       // This is only safe to do because the lock file ensures that their writers
-       // are no longer active.
+       // Clean up any remaining temporary directories created by old versions
+       // (before 1.16), as well as partially extracted directories (indicated by
+       // DownloadDirPartialError, usually because of a .partial file). This is only
+       // safe to do because the lock file ensures that their writers are no longer
+       // active.
        parentDir := filepath.Dir(dir)
        tmpPrefix := filepath.Base(dir) + ".tmp-"
        if old, err := filepath.Glob(filepath.Join(parentDir, tmpPrefix+"*")); err == nil {
@@ -116,88 +117,44 @@ func download(ctx context.Context, mod module.Version) (dir string, err error) {
        if err != nil {
                return "", err
        }
-       if err := os.Remove(partialPath); err != nil && !os.IsNotExist(err) {
-               return "", err
-       }
 
-       // Extract the module zip directory.
+       // Extract the module zip directory at its final location.
        //
-       // 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.
+       // To prevent other processes from reading the directory if we crash,
+       // create a .partial file before extracting the directory, and delete
+       // the .partial file afterward (all while holding the lock).
        //
-       // 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.
+       // Before Go 1.16, we extracted to a temporary directory with a random name
+       // then renamed it into place with os.Rename. On Windows, this failed with
+       // ERROR_ACCESS_DENIED when another process (usually an anti-virus scanner)
+       // opened files in the temporary directory.
        //
-       // 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.
+       // Go 1.14.2 and higher respect .partial files. Older versions may use
+       // partially extracted directories. 'go mod verify' can detect this,
+       // and 'go clean -modcache' can fix it.
        if err := os.MkdirAll(parentDir, 0777); err != nil {
                return "", err
        }
-
-       ctx, span = trace.StartSpan(ctx, "unzip "+zipfile)
-       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 := 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
        }
-       defer span.Done()
 
        if !cfg.ModCacheRW {
-               // Make dir read-only only *after* renaming it.
-               // os.Rename was observed to fail for read-only directories on macOS.
                makeDirsReadOnly(dir)
        }
        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
diff --git a/src/cmd/go/testdata/script/mod_concurrent_unzipinplace.txt b/src/cmd/go/testdata/script/mod_concurrent_unzipinplace.txt
deleted file mode 100644 (file)
index 473be71..0000000
+++ /dev/null
@@ -1,17 +0,0 @@
-# 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
index bb9c58889605b0961d1f03955b928ed4d7cd251f..caf105c6e5feb3669532127b4a20181f8855cf8f 100644 (file)
@@ -1,27 +1,18 @@
 # 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.
+# Before Go 1.16, we extracted each module zip to a temporary directory with
+# a random name, then renamed that into place with os.Rename. On Windows,
+# this failed with ERROR_ACCESS_DENIED when another process (usually an
+# anti-virus scanner) opened files in the temporary directory. This test
+# simulates that behavior, verifying golang.org/issue/36568.
 #
-# 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.
+# Since 1.16, we extract to the final directory, but we create a .partial file
+# so that if we crash, other processes know the directory is incomplete.
 
 [!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 --
index 8d319701608661c1b205f3ab23550ea61e0370ff..0aab60ddaf00f84c24f19073af65dc0bec5dc8a2 100644 (file)
@@ -46,7 +46,6 @@ rm $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/go.mod
 
 # '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