[release-branch.go1.14] cmd/go: make module zip extraction more robust
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 #37800
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>
(cherry picked from commit 093049b3709eda7537ece92a2991918cf53782d6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/223147
diff --git a/src/cmd/go/internal/modcmd/verify.go b/src/cmd/go/internal/modcmd/verify.go
index 831e5cf..ac3f135 100644
--- a/src/cmd/go/internal/modcmd/verify.go
+++ b/src/cmd/go/internal/modcmd/verify.go
@@ -6,6 +6,7 @@
import (
"bytes"
+ "errors"
"fmt"
"io/ioutil"
"os"
@@ -67,12 +68,10 @@
_, 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 @@
}
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 @@
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)
diff --git a/src/cmd/go/internal/modfetch/cache.go b/src/cmd/go/internal/modfetch/cache.go
index 947192b..d6ff068 100644
--- a/src/cmd/go/internal/modfetch/cache.go
+++ b/src/cmd/go/internal/modfetch/cache.go
@@ -7,6 +7,7 @@
import (
"bytes"
"encoding/json"
+ "errors"
"fmt"
"io"
"io/ioutil"
@@ -56,8 +57,11 @@
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 @@
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) {
diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go
index 54fbd92..aadf883 100644
--- a/src/cmd/go/internal/modfetch/fetch.go
+++ b/src/cmd/go/internal/modfetch/fetch.go
@@ -22,6 +22,7 @@
"cmd/go/internal/lockedfile"
"cmd/go/internal/par"
"cmd/go/internal/renameio"
+ "cmd/go/internal/robustio"
"golang.org/x/mod/module"
"golang.org/x/mod/sumdb/dirhash"
@@ -45,24 +46,27 @@
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,
@@ -70,22 +74,24 @@
// 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)
@@ -95,6 +101,19 @@
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
@@ -102,11 +121,11 @@
// the entire directory (e.g. as an attempt to prune out file corruption)
// the module cache will still be left in a recoverable state.
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 {
@@ -116,11 +135,11 @@
if err := modzip.Unzip(tmpDir, mod, zipfile); err != nil {
fmt.Fprintf(os.Stderr, "-> %s\n", err)
- return err
+ return "", err
}
- if err := os.Rename(tmpDir, dir); err != nil {
- return err
+ if err := robustio.Rename(tmpDir, dir); err != nil {
+ return "", err
}
if !cfg.ModCacheRW {
@@ -128,7 +147,7 @@
// os.Rename was observed to fail for read-only directories on macOS.
makeDirsReadOnly(dir)
}
- return nil
+ return dir, nil
}
var downloadZipCache par.Cache
diff --git a/src/cmd/go/internal/modload/build.go b/src/cmd/go/internal/modload/build.go
index 6fa47d7..acd8f38 100644
--- a/src/cmd/go/internal/modload/build.go
+++ b/src/cmd/go/internal/modload/build.go
@@ -148,9 +148,7 @@
}
dir, err := modfetch.DownloadDir(mod)
if err == nil {
- if info, err := os.Stat(dir); err == nil && info.IsDir() {
- m.Dir = dir
- }
+ m.Dir = dir
}
}
}
diff --git a/src/cmd/go/internal/robustio/robustio_flaky.go b/src/cmd/go/internal/robustio/robustio_flaky.go
index e57c8c7..d4cb7e6 100644
--- a/src/cmd/go/internal/robustio/robustio_flaky.go
+++ b/src/cmd/go/internal/robustio/robustio_flaky.go
@@ -15,7 +15,7 @@
"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
index 0000000..b035382
--- /dev/null
+++ b/src/cmd/go/testdata/script/mod_download_partial.txt
@@ -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 --
