]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: make module zip extraction more robust
authorJay Conrod <jayconrod@google.com>
Fri, 28 Feb 2020 21:31:19 +0000 (16:31 -0500)
committerJay Conrod <jayconrod@google.com>
Wed, 11 Mar 2020 17:31:00 +0000 (17:31 +0000)
Currently, we extract module zip files to temporary directories, then
atomically rename them into place. On Windows, this can fail with
ERROR_ACCESS_DENIED if another process (antivirus) has files open
before the rename. In CL 220978, we repeated the rename operation in a
loop over 500 ms, but this didn't solve the problem for everyone.

A better solution will extract module zip files to their permanent
locations in the cache and will keep a ".partial" marker file,
indicating when a module hasn't been fully extracted (CL 221157).
This approach is not safe if current versions of Go access the module
cache concurrently, since the module directory is detected with a
single os.Stat.

In the interim, this CL makes two changes:

1. Flaky file system operations are repeated over 2000 ms to reduce
the chance of this error occurring.
2. cmd/go will now check for .partial files created by future
versions. If a .partial file is found, it will lock the lock file,
then remove the .partial file and directory if needed.

After some time has passed and Go versions lacking this CL are no
longer supported, we can start extracting module zip files in place.

Updates #36568

Change-Id: I467ee11aa59a90b63cf0e3e761c4fec89d57d3b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/221820
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
src/cmd/go/internal/modcmd/verify.go
src/cmd/go/internal/modfetch/cache.go
src/cmd/go/internal/modfetch/fetch.go
src/cmd/go/internal/modload/build.go
src/cmd/go/internal/robustio/robustio_flaky.go
src/cmd/go/testdata/script/mod_download_partial.txt [new file with mode: 0644]

index 831e5cf85bba46510de1d5e4b2b7fac7690d04d2..ac3f1351c879eb0cc4a8eda938ce91d5b78ac8a6 100644 (file)
@@ -6,6 +6,7 @@ package modcmd
 
 import (
        "bytes"
+       "errors"
        "fmt"
        "io/ioutil"
        "os"
@@ -67,12 +68,10 @@ func verifyMod(mod module.Version) bool {
                _, zipErr = os.Stat(zip)
        }
        dir, dirErr := modfetch.DownloadDir(mod)
-       if dirErr == nil {
-               _, dirErr = os.Stat(dir)
-       }
        data, err := ioutil.ReadFile(zip + "hash")
        if err != nil {
-               if zipErr != nil && os.IsNotExist(zipErr) && dirErr != nil && os.IsNotExist(dirErr) {
+               if zipErr != nil && errors.Is(zipErr, os.ErrNotExist) &&
+                       dirErr != nil && errors.Is(dirErr, os.ErrNotExist) {
                        // Nothing downloaded yet. Nothing to verify.
                        return true
                }
@@ -81,7 +80,7 @@ func verifyMod(mod module.Version) bool {
        }
        h := string(bytes.TrimSpace(data))
 
-       if zipErr != nil && os.IsNotExist(zipErr) {
+       if zipErr != nil && errors.Is(zipErr, os.ErrNotExist) {
                // ok
        } else {
                hZ, err := dirhash.HashZip(zip, dirhash.DefaultHash)
@@ -93,7 +92,7 @@ func verifyMod(mod module.Version) bool {
                        ok = false
                }
        }
-       if dirErr != nil && os.IsNotExist(dirErr) {
+       if dirErr != nil && errors.Is(dirErr, os.ErrNotExist) {
                // ok
        } else {
                hD, err := dirhash.HashDir(dir, mod.Path+"@"+mod.Version, dirhash.DefaultHash)
index 947192bd83ee387bc94f23a8323cd5fb874e4454..d6ff068e7bccc9222e090381e381b51a7953ac14 100644 (file)
@@ -7,6 +7,7 @@ package modfetch
 import (
        "bytes"
        "encoding/json"
+       "errors"
        "fmt"
        "io"
        "io/ioutil"
@@ -56,8 +57,11 @@ func CachePath(m module.Version, suffix string) (string, error) {
        return filepath.Join(dir, encVer+"."+suffix), nil
 }
 
-// DownloadDir returns the directory to which m should be downloaded.
-// Note that the directory may not yet exist.
+// DownloadDir returns the directory to which m should have been downloaded.
+// An error will be returned if the module path or version cannot be escaped.
+// An error satisfying errors.Is(err, os.ErrNotExist) will be returned
+// along with the directory if the directory does not exist or if the directory
+// is not completely populated.
 func DownloadDir(m module.Version) (string, error) {
        if PkgMod == "" {
                return "", fmt.Errorf("internal error: modfetch.PkgMod not set")
@@ -76,9 +80,39 @@ func DownloadDir(m module.Version) (string, error) {
        if err != nil {
                return "", err
        }
-       return filepath.Join(PkgMod, enc+"@"+encVer), nil
+
+       dir := filepath.Join(PkgMod, enc+"@"+encVer)
+       if fi, err := os.Stat(dir); os.IsNotExist(err) {
+               return dir, err
+       } else if err != nil {
+               return dir, &DownloadDirPartialError{dir, err}
+       } else if !fi.IsDir() {
+               return dir, &DownloadDirPartialError{dir, errors.New("not a directory")}
+       }
+       partialPath, err := CachePath(m, "partial")
+       if err != nil {
+               return dir, err
+       }
+       if _, err := os.Stat(partialPath); err == nil {
+               return dir, &DownloadDirPartialError{dir, errors.New("not completely extracted")}
+       } else if !os.IsNotExist(err) {
+               return dir, err
+       }
+       return dir, nil
+}
+
+// DownloadDirPartialError is returned by DownloadDir if a module directory
+// exists but was not completely populated.
+//
+// DownloadDirPartialError is equivalent to os.ErrNotExist.
+type DownloadDirPartialError struct {
+       Dir string
+       Err error
 }
 
+func (e *DownloadDirPartialError) Error() string     { return fmt.Sprintf("%s: %v", e.Dir, e.Err) }
+func (e *DownloadDirPartialError) Is(err error) bool { return err == os.ErrNotExist }
+
 // 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) {
index e2c463a6855b5d44cc2e306e3d686cb6ca7ce228..5787e14aa071c87fa3ac5214beb08d7b73f273f4 100644 (file)
@@ -46,24 +46,27 @@ func Download(mod module.Version) (dir string, err error) {
                err error
        }
        c := downloadCache.Do(mod, func() interface{} {
-               dir, err := DownloadDir(mod)
+               dir, err := download(mod)
                if err != nil {
                        return cached{"", err}
                }
-               if err := download(mod, dir); err != nil {
-                       return cached{"", err}
-               }
                checkMod(mod)
                return cached{dir, nil}
        }).(cached)
        return c.dir, c.err
 }
 
-func download(mod module.Version, dir string) (err error) {
-       // If the directory exists, the module has already been extracted.
-       fi, err := os.Stat(dir)
-       if err == nil && fi.IsDir() {
-               return nil
+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.
+       dir, err = DownloadDir(mod)
+       if err == nil {
+               return dir, nil
+       } else if dir == "" || !errors.Is(err, os.ErrNotExist) {
+               return "", err
        }
 
        // To avoid cluttering the cache with extraneous files,
@@ -71,22 +74,24 @@ func download(mod module.Version, dir string) (err error) {
        // Invoke DownloadZip before locking the file.
        zipfile, err := DownloadZip(mod)
        if err != nil {
-               return err
+               return "", err
        }
 
        unlock, err := lockVersion(mod)
        if err != nil {
-               return err
+               return "", err
        }
        defer unlock()
 
        // Check whether the directory was populated while we were waiting on the lock.
-       fi, err = os.Stat(dir)
-       if err == nil && fi.IsDir() {
-               return nil
+       _, dirErr := DownloadDir(mod)
+       if dirErr == nil {
+               return dir, nil
        }
+       _, dirExists := dirErr.(*DownloadDirPartialError)
 
-       // Clean up any remaining temporary directories from previous runs.
+       // 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.
        parentDir := filepath.Dir(dir)
@@ -96,6 +101,19 @@ func download(mod module.Version, dir string) (err error) {
                        RemoveAll(path) // best effort
                }
        }
+       if dirExists {
+               if err := RemoveAll(dir); err != nil {
+                       return "", err
+               }
+       }
+
+       partialPath, err := CachePath(mod, "partial")
+       if err != nil {
+               return "", err
+       }
+       if err := os.Remove(partialPath); err != nil && !os.IsNotExist(err) {
+               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
@@ -106,11 +124,11 @@ func download(mod module.Version, dir string) (err error) {
        // open files in the temporary directory (antivirus, search indexers, etc.)
        // can cause os.Rename to fail with ERROR_ACCESS_DENIED.
        if err := os.MkdirAll(parentDir, 0777); err != nil {
-               return err
+               return "", err
        }
        tmpDir, err := ioutil.TempDir(parentDir, tmpPrefix)
        if err != nil {
-               return err
+               return "", err
        }
        defer func() {
                if err != nil {
@@ -120,11 +138,11 @@ func download(mod module.Version, dir string) (err error) {
 
        if err := modzip.Unzip(tmpDir, mod, zipfile); err != nil {
                fmt.Fprintf(os.Stderr, "-> %s\n", err)
-               return err
+               return "", err
        }
 
        if err := robustio.Rename(tmpDir, dir); err != nil {
-               return err
+               return "", err
        }
 
        if !cfg.ModCacheRW {
@@ -132,7 +150,7 @@ func download(mod module.Version, dir string) (err error) {
                // os.Rename was observed to fail for read-only directories on macOS.
                makeDirsReadOnly(dir)
        }
-       return nil
+       return dir, nil
 }
 
 var downloadZipCache par.Cache
index 454dbf28cfca3493458b43a1d493f93491d8d584..5f8a2e7e056501178fba7624db771a062b3b36b3 100644 (file)
@@ -148,9 +148,7 @@ func moduleInfo(m module.Version, fromBuildList bool) *modinfo.ModulePublic {
                        }
                        dir, err := modfetch.DownloadDir(mod)
                        if err == nil {
-                               if info, err := os.Stat(dir); err == nil && info.IsDir() {
-                                       m.Dir = dir
-                               }
+                               m.Dir = dir
                        }
                }
        }
index e57c8c74c4c6a4c97484cd9ff6dcc8773a2e3362..d4cb7e6457fc05d8d548f12399b390258bfb4f80 100644 (file)
@@ -15,7 +15,7 @@ import (
        "time"
 )
 
-const arbitraryTimeout = 500 * time.Millisecond
+const arbitraryTimeout = 2000 * time.Millisecond
 
 // retry retries ephemeral errors from f up to an arbitrary timeout
 // to work around filesystem flakiness on Windows and Darwin.
diff --git a/src/cmd/go/testdata/script/mod_download_partial.txt b/src/cmd/go/testdata/script/mod_download_partial.txt
new file mode 100644 (file)
index 0000000..b035382
--- /dev/null
@@ -0,0 +1,54 @@
+# Download a module
+go mod download -modcacherw rsc.io/quote
+exists $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/go.mod
+
+# 'go mod verify' should fail if we delete a file.
+go mod verify
+rm $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/go.mod
+! go mod verify
+
+# Create a .partial file to simulate an failure extracting the zip file.
+cp empty $GOPATH/pkg/mod/cache/download/rsc.io/quote/@v/v1.5.2.partial
+
+# 'go mod verify' should not fail, since the module hasn't been completely
+# ingested into the cache.
+go mod verify
+
+# 'go list' should not load packages from the directory.
+# NOTE: the message "directory $dir outside available modules" is reported
+# for directories not in the main module, active modules in the module cache,
+# or local replacements. In this case, the directory is in the right place,
+# but it's incomplete, so 'go list' acts as if it's not an active module.
+! go list $GOPATH/pkg/mod/rsc.io/quote@v1.5.2
+stderr 'outside available modules'
+
+# 'go list -m' should not print the directory.
+go list -m -f '{{.Dir}}' rsc.io/quote
+! stdout .
+
+# 'go mod download' should re-extract the module and remove the .partial file.
+go mod download -modcacherw rsc.io/quote
+! exists $GOPATH/pkg/mod/cache/download/rsc.io/quote/@v/v1.5.2.partial
+exists $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/go.mod
+
+# 'go list' should succeed.
+go list $GOPATH/pkg/mod/rsc.io/quote@v1.5.2
+stdout '^rsc.io/quote$'
+
+# 'go list -m' should print the directory.
+go list -m -f '{{.Dir}}' rsc.io/quote
+stdout 'pkg[/\\]mod[/\\]rsc.io[/\\]quote@v1.5.2'
+
+# go mod verify should fail if we delete a file.
+go mod verify
+rm $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/go.mod
+! go mod verify
+
+-- go.mod --
+module m
+
+go 1.14
+
+require rsc.io/quote v1.5.2
+
+-- empty --