Commit 25a2b98f authored by Bryan C. Mills's avatar Bryan C. Mills

cmd/go: factor the I/O-retry logic out of renameio

Factor the try-on-failure variants are now in the package
cmd/go/internal/robustio.

Add to them a RemoveAll variant using the same retry loop,
and use it to attempt to address the observed flakes in
TestLinkXImportPathEscape.

Fixes #19491
Updates #25965
Updates #28387
Updates #32188

Change-Id: I9db1a0c7537b8aaadccab1b9eca734595668ba29
Reviewed-on: https://go-review.googlesource.com/c/go/+/181541
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: default avatarAlex Brainman <alex.brainman@gmail.com>
Reviewed-by: default avatarJay Conrod <jayconrod@google.com>
Reviewed-by: default avatarRuss Cox <rsc@golang.org>
parent 55d31e16
...@@ -29,6 +29,7 @@ import ( ...@@ -29,6 +29,7 @@ import (
"cmd/go/internal/cache" "cmd/go/internal/cache"
"cmd/go/internal/cfg" "cmd/go/internal/cfg"
"cmd/go/internal/robustio"
"cmd/internal/sys" "cmd/internal/sys"
) )
...@@ -685,7 +686,7 @@ func (tg *testgoData) creatingTemp(path string) { ...@@ -685,7 +686,7 @@ func (tg *testgoData) creatingTemp(path string) {
if tg.wd != "" && !filepath.IsAbs(path) { if tg.wd != "" && !filepath.IsAbs(path) {
path = filepath.Join(tg.pwd(), path) path = filepath.Join(tg.pwd(), path)
} }
tg.must(os.RemoveAll(path)) tg.must(robustio.RemoveAll(path))
tg.temps = append(tg.temps, path) tg.temps = append(tg.temps, path)
} }
...@@ -887,7 +888,7 @@ func removeAll(dir string) error { ...@@ -887,7 +888,7 @@ func removeAll(dir string) error {
} }
return nil return nil
}) })
return os.RemoveAll(dir) return robustio.RemoveAll(dir)
} }
// failSSH puts an ssh executable in the PATH that always fails. // failSSH puts an ssh executable in the PATH that always fails.
...@@ -1181,7 +1182,7 @@ func testMove(t *testing.T, vcs, url, base, config string) { ...@@ -1181,7 +1182,7 @@ func testMove(t *testing.T, vcs, url, base, config string) {
case "svn": case "svn":
// SVN doesn't believe in text files so we can't just edit the config. // SVN doesn't believe in text files so we can't just edit the config.
// Check out a different repo into the wrong place. // Check out a different repo into the wrong place.
tg.must(os.RemoveAll(tg.path("src/code.google.com/p/rsc-svn"))) tg.must(robustio.RemoveAll(tg.path("src/code.google.com/p/rsc-svn")))
tg.run("get", "-d", "-u", "code.google.com/p/rsc-svn2/trunk") tg.run("get", "-d", "-u", "code.google.com/p/rsc-svn2/trunk")
tg.must(os.Rename(tg.path("src/code.google.com/p/rsc-svn2"), tg.path("src/code.google.com/p/rsc-svn"))) tg.must(os.Rename(tg.path("src/code.google.com/p/rsc-svn2"), tg.path("src/code.google.com/p/rsc-svn")))
default: default:
...@@ -1693,7 +1694,7 @@ func TestInstalls(t *testing.T) { ...@@ -1693,7 +1694,7 @@ func TestInstalls(t *testing.T) {
goarch := strings.TrimSpace(tg.getStdout()) goarch := strings.TrimSpace(tg.getStdout())
tg.setenv("GOARCH", goarch) tg.setenv("GOARCH", goarch)
fixbin := filepath.Join(goroot, "pkg", "tool", goos+"_"+goarch, "fix") + exeSuffix fixbin := filepath.Join(goroot, "pkg", "tool", goos+"_"+goarch, "fix") + exeSuffix
tg.must(os.RemoveAll(fixbin)) tg.must(robustio.RemoveAll(fixbin))
tg.run("install", "cmd/fix") tg.run("install", "cmd/fix")
tg.wantExecutable(fixbin, "did not install cmd/fix to $GOROOT/pkg/tool") tg.wantExecutable(fixbin, "did not install cmd/fix to $GOROOT/pkg/tool")
tg.must(os.Remove(fixbin)) tg.must(os.Remove(fixbin))
...@@ -2065,13 +2066,13 @@ func TestDefaultGOPATHGet(t *testing.T) { ...@@ -2065,13 +2066,13 @@ func TestDefaultGOPATHGet(t *testing.T) {
tg.grepStderr("created GOPATH="+regexp.QuoteMeta(tg.path("home/go"))+"; see 'go help gopath'", "did not create GOPATH") tg.grepStderr("created GOPATH="+regexp.QuoteMeta(tg.path("home/go"))+"; see 'go help gopath'", "did not create GOPATH")
// no warning if directory already exists // no warning if directory already exists
tg.must(os.RemoveAll(tg.path("home/go"))) tg.must(robustio.RemoveAll(tg.path("home/go")))
tg.tempDir("home/go") tg.tempDir("home/go")
tg.run("get", "github.com/golang/example/hello") tg.run("get", "github.com/golang/example/hello")
tg.grepStderrNot(".", "expected no output on standard error") tg.grepStderrNot(".", "expected no output on standard error")
// error if $HOME/go is a file // error if $HOME/go is a file
tg.must(os.RemoveAll(tg.path("home/go"))) tg.must(robustio.RemoveAll(tg.path("home/go")))
tg.tempFile("home/go", "") tg.tempFile("home/go", "")
tg.runFail("get", "github.com/golang/example/hello") tg.runFail("get", "github.com/golang/example/hello")
tg.grepStderr(`mkdir .*[/\\]go: .*(not a directory|cannot find the path)`, "expected error because $HOME/go is a file") tg.grepStderr(`mkdir .*[/\\]go: .*(not a directory|cannot find the path)`, "expected error because $HOME/go is a file")
...@@ -2872,7 +2873,7 @@ func TestCgoDependsOnSyscall(t *testing.T) { ...@@ -2872,7 +2873,7 @@ func TestCgoDependsOnSyscall(t *testing.T) {
files, err := filepath.Glob(filepath.Join(runtime.GOROOT(), "pkg", "*_race")) files, err := filepath.Glob(filepath.Join(runtime.GOROOT(), "pkg", "*_race"))
tg.must(err) tg.must(err)
for _, file := range files { for _, file := range files {
tg.check(os.RemoveAll(file)) tg.check(robustio.RemoveAll(file))
} }
tg.tempFile("src/foo/foo.go", ` tg.tempFile("src/foo/foo.go", `
package foo package foo
...@@ -3925,10 +3926,10 @@ func TestGoGetDomainRoot(t *testing.T) { ...@@ -3925,10 +3926,10 @@ func TestGoGetDomainRoot(t *testing.T) {
tg.run("get", "go-get-issue-9357.appspot.com") tg.run("get", "go-get-issue-9357.appspot.com")
tg.run("get", "-u", "go-get-issue-9357.appspot.com") tg.run("get", "-u", "go-get-issue-9357.appspot.com")
tg.must(os.RemoveAll(tg.path("src/go-get-issue-9357.appspot.com"))) tg.must(robustio.RemoveAll(tg.path("src/go-get-issue-9357.appspot.com")))
tg.run("get", "go-get-issue-9357.appspot.com") tg.run("get", "go-get-issue-9357.appspot.com")
tg.must(os.RemoveAll(tg.path("src/go-get-issue-9357.appspot.com"))) tg.must(robustio.RemoveAll(tg.path("src/go-get-issue-9357.appspot.com")))
tg.run("get", "-u", "go-get-issue-9357.appspot.com") tg.run("get", "-u", "go-get-issue-9357.appspot.com")
} }
...@@ -4513,8 +4514,9 @@ func TestLinkXImportPathEscape(t *testing.T) { ...@@ -4513,8 +4514,9 @@ func TestLinkXImportPathEscape(t *testing.T) {
tg := testgo(t) tg := testgo(t)
defer tg.cleanup() defer tg.cleanup()
tg.parallel() tg.parallel()
tg.makeTempdir()
tg.setenv("GOPATH", filepath.Join(tg.pwd(), "testdata")) tg.setenv("GOPATH", filepath.Join(tg.pwd(), "testdata"))
exe := "./linkx" + exeSuffix exe := tg.path("linkx" + exeSuffix)
tg.creatingTemp(exe) tg.creatingTemp(exe)
tg.run("build", "-o", exe, "-ldflags", "-X=my.pkg.Text=linkXworked", "my.pkg/main") tg.run("build", "-o", exe, "-ldflags", "-X=my.pkg.Text=linkXworked", "my.pkg/main")
out, err := exec.Command(exe).CombinedOutput() out, err := exec.Command(exe).CombinedOutput()
...@@ -4750,7 +4752,7 @@ func TestExecutableGOROOT(t *testing.T) { ...@@ -4750,7 +4752,7 @@ func TestExecutableGOROOT(t *testing.T) {
check(t, symGoTool, newRoot) check(t, symGoTool, newRoot)
}) })
tg.must(os.RemoveAll(tg.path("new/pkg"))) tg.must(robustio.RemoveAll(tg.path("new/pkg")))
// Binaries built in the new tree should report the // Binaries built in the new tree should report the
// new tree when they call runtime.GOROOT. // new tree when they call runtime.GOROOT.
...@@ -5101,7 +5103,7 @@ func TestExecBuildX(t *testing.T) { ...@@ -5101,7 +5103,7 @@ func TestExecBuildX(t *testing.T) {
if len(matches) == 0 { if len(matches) == 0 {
t.Fatal("no WORK directory") t.Fatal("no WORK directory")
} }
tg.must(os.RemoveAll(matches[1])) tg.must(robustio.RemoveAll(matches[1]))
} }
func TestParallelNumber(t *testing.T) { func TestParallelNumber(t *testing.T) {
......
...@@ -12,6 +12,8 @@ import ( ...@@ -12,6 +12,8 @@ import (
"path/filepath" "path/filepath"
"strings" "strings"
"testing" "testing"
"cmd/go/internal/robustio"
) )
func TestAbsolutePath(t *testing.T) { func TestAbsolutePath(t *testing.T) {
...@@ -21,7 +23,7 @@ func TestAbsolutePath(t *testing.T) { ...@@ -21,7 +23,7 @@ func TestAbsolutePath(t *testing.T) {
if err != nil { if err != nil {
t.Fatal(err) t.Fatal(err)
} }
defer os.RemoveAll(tmp) defer robustio.RemoveAll(tmp)
file := filepath.Join(tmp, "a.go") file := filepath.Join(tmp, "a.go")
err = ioutil.WriteFile(file, []byte{}, 0644) err = ioutil.WriteFile(file, []byte{}, 0644)
......
...@@ -12,6 +12,8 @@ import ( ...@@ -12,6 +12,8 @@ import (
"os" "os"
"path/filepath" "path/filepath"
"strconv" "strconv"
"cmd/go/internal/robustio"
) )
const patternSuffix = ".tmp" const patternSuffix = ".tmp"
...@@ -61,7 +63,7 @@ func WriteToFile(filename string, data io.Reader, perm os.FileMode) (err error) ...@@ -61,7 +63,7 @@ func WriteToFile(filename string, data io.Reader, perm os.FileMode) (err error)
return err return err
} }
return rename(f.Name(), filename) return robustio.Rename(f.Name(), filename)
} }
// ReadFile is like ioutil.ReadFile, but on Windows retries spurious errors that // ReadFile is like ioutil.ReadFile, but on Windows retries spurious errors that
...@@ -74,7 +76,7 @@ func WriteToFile(filename string, data io.Reader, perm os.FileMode) (err error) ...@@ -74,7 +76,7 @@ func WriteToFile(filename string, data io.Reader, perm os.FileMode) (err error)
// - syscall.ERROR_FILE_NOT_FOUND // - syscall.ERROR_FILE_NOT_FOUND
// - internal/syscall/windows.ERROR_SHARING_VIOLATION // - internal/syscall/windows.ERROR_SHARING_VIOLATION
func ReadFile(filename string) ([]byte, error) { func ReadFile(filename string) ([]byte, error) {
return readFile(filename) return robustio.ReadFile(filename)
} }
// tempFile creates a new temporary file with given permission bits. // tempFile creates a new temporary file with given permission bits.
......
...@@ -19,6 +19,8 @@ import ( ...@@ -19,6 +19,8 @@ import (
"syscall" "syscall"
"testing" "testing"
"time" "time"
"cmd/go/internal/robustio"
) )
func TestConcurrentReadsAndWrites(t *testing.T) { func TestConcurrentReadsAndWrites(t *testing.T) {
...@@ -58,7 +60,7 @@ func TestConcurrentReadsAndWrites(t *testing.T) { ...@@ -58,7 +60,7 @@ func TestConcurrentReadsAndWrites(t *testing.T) {
chunk := buf[offset*8 : (offset+chunkWords)*8] chunk := buf[offset*8 : (offset+chunkWords)*8]
if err := WriteFile(path, chunk, 0666); err == nil { if err := WriteFile(path, chunk, 0666); err == nil {
atomic.AddInt64(&writeSuccesses, 1) atomic.AddInt64(&writeSuccesses, 1)
} else if isEphemeralError(err) { } else if robustio.IsEphemeralError(err) {
var ( var (
errno syscall.Errno errno syscall.Errno
dup bool dup bool
...@@ -74,10 +76,10 @@ func TestConcurrentReadsAndWrites(t *testing.T) { ...@@ -74,10 +76,10 @@ func TestConcurrentReadsAndWrites(t *testing.T) {
} }
time.Sleep(time.Duration(rand.Intn(100)) * time.Microsecond) time.Sleep(time.Duration(rand.Intn(100)) * time.Microsecond)
data, err := ioutil.ReadFile(path) data, err := ReadFile(path)
if err == nil { if err == nil {
atomic.AddInt64(&readSuccesses, 1) atomic.AddInt64(&readSuccesses, 1)
} else if isEphemeralError(err) { } else if robustio.IsEphemeralError(err) {
var ( var (
errno syscall.Errno errno syscall.Errno
dup bool dup bool
......
// Copyright 2019 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
// Package robustio wraps I/O functions that are prone to failure on Windows,
// transparently retrying errors up to an arbitrary timeout.
//
// Errors are classified heuristically and retries are bounded, so the functions
// in this package do not completely eliminate spurious errors. However, they do
// significantly reduce the rate of failure in practice.
//
// If so, the error will likely wrap one of:
// The functions in this package do not completely eliminate spurious errors,
// but substantially reduce their rate of occurrence in practice.
package robustio
// Rename is like os.Rename, but on Windows retries errors that may occur if the
// file is concurrently read or overwritten.
//
// (See golang.org/issue/31247 and golang.org/issue/32188.)
func Rename(oldpath, newpath string) error {
return rename(oldpath, newpath)
}
// ReadFile is like ioutil.ReadFile, but on Windows retries errors that may
// occur if the file is concurrently replaced.
//
// (See golang.org/issue/31247 and golang.org/issue/32188.)
func ReadFile(filename string) ([]byte, error) {
return readFile(filename)
}
// RemoveAll is like os.RemoveAll, but on Windows retries errors that may occur
// if an executable file in the directory has recently been executed.
//
// (See golang.org/issue/19491.)
func RemoveAll(path string) error {
return removeAll(path)
}
// IsEphemeralError reports whether err is one of the errors that the functions
// in this package attempt to mitigate.
//
// Errors considered ephemeral include:
// - syscall.ERROR_ACCESS_DENIED
// - syscall.ERROR_FILE_NOT_FOUND
// - internal/syscall/windows.ERROR_SHARING_VIOLATION
//
// This set may be expanded in the future; programs must not rely on the
// non-ephemerality of any given error.
func IsEphemeralError(err error) bool {
return isEphemeralError(err)
}
...@@ -4,7 +4,7 @@ ...@@ -4,7 +4,7 @@
//+build !windows //+build !windows
package renameio package robustio
import ( import (
"io/ioutil" "io/ioutil"
...@@ -19,6 +19,10 @@ func readFile(filename string) ([]byte, error) { ...@@ -19,6 +19,10 @@ func readFile(filename string) ([]byte, error) {
return ioutil.ReadFile(filename) return ioutil.ReadFile(filename)
} }
func removeAll(path string) error {
return os.RemoveAll(path)
}
func isEphemeralError(err error) bool { func isEphemeralError(err error) bool {
return false return false
} }
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style // Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file. // license that can be found in the LICENSE file.
package renameio package robustio
import ( import (
"errors" "errors"
...@@ -14,9 +14,10 @@ import ( ...@@ -14,9 +14,10 @@ import (
"time" "time"
) )
const arbitraryTimeout = 500 * time.Millisecond
// retry retries ephemeral errors from f up to an arbitrary timeout // retry retries ephemeral errors from f up to an arbitrary timeout
// to work around spurious filesystem errors on Windows // to work around spurious filesystem errors on Windows
// (see golang.org/issue/31247 and golang.org/issue/32188).
func retry(f func() (err error, mayRetry bool)) error { func retry(f func() (err error, mayRetry bool)) error {
var ( var (
bestErr error bestErr error
...@@ -40,7 +41,7 @@ func retry(f func() (err error, mayRetry bool)) error { ...@@ -40,7 +41,7 @@ func retry(f func() (err error, mayRetry bool)) error {
if start.IsZero() { if start.IsZero() {
start = time.Now() start = time.Now()
} else if d := time.Since(start) + nextSleep; d >= 500*time.Millisecond { } else if d := time.Since(start) + nextSleep; d >= arbitraryTimeout {
break break
} }
time.Sleep(nextSleep) time.Sleep(nextSleep)
...@@ -61,8 +62,6 @@ func retry(f func() (err error, mayRetry bool)) error { ...@@ -61,8 +62,6 @@ func retry(f func() (err error, mayRetry bool)) error {
// //
// Empirical error rates with MoveFileEx are lower under modest concurrency, so // Empirical error rates with MoveFileEx are lower under modest concurrency, so
// for now we're sticking with what the os package already provides. // for now we're sticking with what the os package already provides.
//
// TODO(bcmills): For Go 1.14, should we try changing os.Rename itself to do this?
func rename(oldpath, newpath string) (err error) { func rename(oldpath, newpath string) (err error) {
return retry(func() (err error, mayRetry bool) { return retry(func() (err error, mayRetry bool) {
err = os.Rename(oldpath, newpath) err = os.Rename(oldpath, newpath)
...@@ -71,8 +70,6 @@ func rename(oldpath, newpath string) (err error) { ...@@ -71,8 +70,6 @@ func rename(oldpath, newpath string) (err error) {
} }
// readFile is like ioutil.ReadFile, but retries ephemeral errors. // readFile is like ioutil.ReadFile, but retries ephemeral errors.
//
// TODO(bcmills): For Go 1.14, should we try changing ioutil.ReadFile itself to do this?
func readFile(filename string) ([]byte, error) { func readFile(filename string) ([]byte, error) {
var b []byte var b []byte
err := retry(func() (err error, mayRetry bool) { err := retry(func() (err error, mayRetry bool) {
...@@ -86,6 +83,13 @@ func readFile(filename string) ([]byte, error) { ...@@ -86,6 +83,13 @@ func readFile(filename string) ([]byte, error) {
return b, err return b, err
} }
func removeAll(path string) error {
return retry(func() (err error, mayRetry bool) {
err = os.RemoveAll(path)
return err, isEphemeralError(err)
})
}
// isEphemeralError returns true if err may be resolved by waiting. // isEphemeralError returns true if err may be resolved by waiting.
func isEphemeralError(err error) bool { func isEphemeralError(err error) bool {
var errno syscall.Errno var errno syscall.Errno
......
...@@ -27,6 +27,7 @@ import ( ...@@ -27,6 +27,7 @@ import (
"cmd/go/internal/cfg" "cmd/go/internal/cfg"
"cmd/go/internal/imports" "cmd/go/internal/imports"
"cmd/go/internal/par" "cmd/go/internal/par"
"cmd/go/internal/robustio"
"cmd/go/internal/txtar" "cmd/go/internal/txtar"
"cmd/go/internal/work" "cmd/go/internal/work"
) )
...@@ -388,7 +389,7 @@ func (ts *testScript) cmdCc(neg bool, args []string) { ...@@ -388,7 +389,7 @@ func (ts *testScript) cmdCc(neg bool, args []string) {
var b work.Builder var b work.Builder
b.Init() b.Init()
ts.cmdExec(neg, append(b.GccCmd(".", ""), args...)) ts.cmdExec(neg, append(b.GccCmd(".", ""), args...))
os.RemoveAll(b.WorkDir) robustio.RemoveAll(b.WorkDir)
} }
// cd changes to a different directory. // cd changes to a different directory.
...@@ -669,8 +670,8 @@ func (ts *testScript) cmdRm(neg bool, args []string) { ...@@ -669,8 +670,8 @@ func (ts *testScript) cmdRm(neg bool, args []string) {
} }
for _, arg := range args { for _, arg := range args {
file := ts.mkabs(arg) file := ts.mkabs(arg)
removeAll(file) // does chmod and then attempts rm removeAll(file) // does chmod and then attempts rm
ts.check(os.RemoveAll(file)) // report error ts.check(robustio.RemoveAll(file)) // report error
} }
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment