cmd/go: remove renameio package and its last usage

The last primary usage of renameio was the WriteFile in
modfetch.WriteDiskCache. Because it's not guaranteed that the fsync in
WriteDiskCache will eliminate file corruption, and it slows down tests
on Macs significantly, inline that last usage, removing the fsync.

Also, remove the uses of renameio.Pattern. The ziphash file is no
longer written to a temporary location before being copied to its
final location, so that usage can just be cut. The remaining use is
for the zipfile . Remove the first because the files are no longer
written using the pattern anyway, so that the pattern variable has no
effect. Replace it with a local pattern variable that is also passed
to os.CreateTemp.

Change-Id: Icf3adabf2a26c37b82afa1d07f821a46b30d69ce
Reviewed-on: https://go-review.googlesource.com/c/go/+/301889
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

src/cmd/go/internal/modfetch/cache.go

@@ -11,8 +11,10 @@ import (
 	"fmt"
 	"io"
 	"io/fs"
+	"math/rand"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"sync"
 
@@ -21,7 +23,7 @@ import (
 	"cmd/go/internal/lockedfile"
 	"cmd/go/internal/modfetch/codehost"
 	"cmd/go/internal/par"
-	"cmd/go/internal/renameio"
+	"cmd/go/internal/robustio"
 
 	"golang.org/x/mod/module"
 	"golang.org/x/mod/semver"
@@ -543,7 +545,7 @@ func readDiskCache(path, rev, suffix string) (file string, data []byte, err erro
 	if err != nil {
 		return "", nil, errNotCached
 	}
-	data, err = renameio.ReadFile(file)
+	data, err = robustio.ReadFile(file)
 	if err != nil {
 		return file, nil, errNotCached
 	}
@@ -580,7 +582,29 @@ func writeDiskCache(file string, data []byte) error {
 		return err
 	}
 
-	if err := renameio.WriteFile(file, data, 0666); err != nil {
+	// Write the file to a temporary location, and then rename it to its final
+	// path to reduce the likelihood of a corrupt file existing at that final path.
+	f, err := tempFile(filepath.Dir(file), filepath.Base(file), 0666)
+	if err != nil {
+		return err
+	}
+	defer func() {
+		// Only call os.Remove on f.Name() if we failed to rename it: otherwise,
+		// some other process may have created a new file with the same name after
+		// the rename completed.
+		if err != nil {
+			f.Close()
+			os.Remove(f.Name())
+		}
+	}()
+
+	if _, err := f.Write(data); err != nil {
+		return err
+	}
+	if err := f.Close(); err != nil {
+		return err
+	}
+	if err := robustio.Rename(f.Name(), file); err != nil {
 		return err
 	}
 
@@ -590,6 +614,19 @@ func writeDiskCache(file string, data []byte) error {
 	return nil
 }
 
+// tempFile creates a new temporary file with given permission bits.
+func tempFile(dir, prefix string, perm fs.FileMode) (f *os.File, err error) {
+	for i := 0; i < 10000; i++ {
+		name := filepath.Join(dir, prefix+strconv.Itoa(rand.Intn(1000000000))+".tmp")
+		f, err = os.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, perm)
+		if os.IsExist(err) {
+			continue
+		}
+		break
+	}
+	return
+}
+
 // rewriteVersionList rewrites the version list in dir
 // after a new *.mod file has been written.
 func rewriteVersionList(dir string) (err error) {

src/cmd/go/internal/modfetch/fetch.go

@@ -24,7 +24,6 @@ import (
 	"cmd/go/internal/cfg"
 	"cmd/go/internal/lockedfile"
 	"cmd/go/internal/par"
-	"cmd/go/internal/renameio"
 	"cmd/go/internal/robustio"
 	"cmd/go/internal/trace"
 
@@ -223,13 +222,12 @@ func downloadZip(ctx context.Context, mod module.Version, zipfile string) (err e
 	// Clean up any remaining tempfiles from previous runs.
 	// This is only safe to do because the lock file ensures that their
 	// writers are no longer active.
-	for _, base := range []string{zipfile, zipfile + "hash"} {
+	tmpPattern := filepath.Base(zipfile) + "*.tmp"
-		if old, err := filepath.Glob(renameio.Pattern(base)); err == nil {
+	if old, err := filepath.Glob(filepath.Join(filepath.Dir(zipfile), tmpPattern)); err == nil {
 		for _, path := range old {
 			os.Remove(path) // best effort
 		}
 	}
-	}
 
 	// If the zip file exists, the ziphash file must have been deleted
 	// or lost after a file system crash. Re-hash the zip without downloading.
@@ -242,7 +240,7 @@ func downloadZip(ctx context.Context, mod module.Version, zipfile string) (err e
 	// contents of the file (by hashing it) before we commit it. Because the file
 	// is zip-compressed, we need an actual file — or at least an io.ReaderAt — to
 	// validate it: we can't just tee the stream as we write it.
-	f, err := os.CreateTemp(filepath.Dir(zipfile), filepath.Base(renameio.Pattern(zipfile)))
+	f, err := os.CreateTemp(filepath.Dir(zipfile), tmpPattern)
 	if err != nil {
 		return err
 	}

src/cmd/go/internal/renameio/renameio.go

@@ -1,88 +0,0 @@
-// Copyright 2018 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 renameio writes files atomically by renaming temporary files.
-package renameio
-
-import (
-	"bytes"
-	"io"
-	"io/fs"
-	"math/rand"
-	"os"
-	"path/filepath"
-	"strconv"
-
-	"cmd/go/internal/robustio"
-)
-
-const patternSuffix = ".tmp"
-
-// Pattern returns a glob pattern that matches the unrenamed temporary files
-// created when writing to filename.
-func Pattern(filename string) string {
-	return filepath.Join(filepath.Dir(filename), filepath.Base(filename)+patternSuffix)
-}
-
-// WriteFile is like os.WriteFile, but first writes data to an arbitrary
-// file in the same directory as filename, then renames it atomically to the
-// final name.
-//
-// That ensures that the final location, if it exists, is always a complete file.
-func WriteFile(filename string, data []byte, perm fs.FileMode) (err error) {
-	f, err := tempFile(filepath.Dir(filename), filepath.Base(filename), perm)
-	if err != nil {
-		return err
-	}
-	defer func() {
-		// Only call os.Remove on f.Name() if we failed to rename it: otherwise,
-		// some other process may have created a new file with the same name after
-		// that.
-		if err != nil {
-			f.Close()
-			os.Remove(f.Name())
-		}
-	}()
-
-	if _, err := io.Copy(f, bytes.NewReader(data)); err != nil {
-		return err
-	}
-	// Sync the file before renaming it: otherwise, after a crash the reader may
-	// observe a 0-length file instead of the actual contents.
-	// See https://golang.org/issue/22397#issuecomment-380831736.
-	if err := f.Sync(); err != nil {
-		return err
-	}
-	if err := f.Close(); err != nil {
-		return err
-	}
-
-	return robustio.Rename(f.Name(), filename)
-}
-
-// ReadFile is like os.ReadFile, but on Windows retries spurious errors that
-// may occur if the file is concurrently replaced.
-//
-// Errors are classified heuristically and retries are bounded, so even this
-// function may occasionally return a spurious error on Windows.
-// If so, the error will likely wrap one of:
-// 	- syscall.ERROR_ACCESS_DENIED
-// 	- syscall.ERROR_FILE_NOT_FOUND
-// 	- internal/syscall/windows.ERROR_SHARING_VIOLATION
-func ReadFile(filename string) ([]byte, error) {
-	return robustio.ReadFile(filename)
-}
-
-// tempFile creates a new temporary file with given permission bits.
-func tempFile(dir, prefix string, perm fs.FileMode) (f *os.File, err error) {
-	for i := 0; i < 10000; i++ {
-		name := filepath.Join(dir, prefix+strconv.Itoa(rand.Intn(1000000000))+patternSuffix)
-		f, err = os.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, perm)
-		if os.IsExist(err) {
-			continue
-		}
-		break
-	}
-	return
-}

src/cmd/go/internal/renameio/renameio_test.go

@@ -1,161 +0,0 @@
-// 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.
-
-//go:build !plan9
-// +build !plan9
-
-package renameio
-
-import (
-	"encoding/binary"
-	"errors"
-	"internal/testenv"
-	"math/rand"
-	"os"
-	"path/filepath"
-	"runtime"
-	"strings"
-	"sync"
-	"sync/atomic"
-	"syscall"
-	"testing"
-	"time"
-
-	"cmd/go/internal/robustio"
-)
-
-func TestConcurrentReadsAndWrites(t *testing.T) {
-	if runtime.GOOS == "darwin" && strings.HasSuffix(testenv.Builder(), "-10_14") {
-		testenv.SkipFlaky(t, 33041)
-	}
-
-	dir, err := os.MkdirTemp("", "renameio")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(dir)
-	path := filepath.Join(dir, "blob.bin")
-
-	const chunkWords = 8 << 10
-	buf := make([]byte, 2*chunkWords*8)
-	for i := uint64(0); i < 2*chunkWords; i++ {
-		binary.LittleEndian.PutUint64(buf[i*8:], i)
-	}
-
-	var attempts int64 = 128
-	if !testing.Short() {
-		attempts *= 16
-	}
-	const parallel = 32
-
-	var sem = make(chan bool, parallel)
-
-	var (
-		writeSuccesses, readSuccesses int64 // atomic
-		writeErrnoSeen, readErrnoSeen sync.Map
-	)
-
-	for n := attempts; n > 0; n-- {
-		sem <- true
-		go func() {
-			defer func() { <-sem }()
-
-			time.Sleep(time.Duration(rand.Intn(100)) * time.Microsecond)
-			offset := rand.Intn(chunkWords)
-			chunk := buf[offset*8 : (offset+chunkWords)*8]
-			if err := WriteFile(path, chunk, 0666); err == nil {
-				atomic.AddInt64(&writeSuccesses, 1)
-			} else if robustio.IsEphemeralError(err) {
-				var (
-					errno syscall.Errno
-					dup   bool
-				)
-				if errors.As(err, &errno) {
-					_, dup = writeErrnoSeen.LoadOrStore(errno, true)
-				}
-				if !dup {
-					t.Logf("ephemeral error: %v", err)
-				}
-			} else {
-				t.Errorf("unexpected error: %v", err)
-			}
-
-			time.Sleep(time.Duration(rand.Intn(100)) * time.Microsecond)
-			data, err := robustio.ReadFile(path)
-			if err == nil {
-				atomic.AddInt64(&readSuccesses, 1)
-			} else if robustio.IsEphemeralError(err) {
-				var (
-					errno syscall.Errno
-					dup   bool
-				)
-				if errors.As(err, &errno) {
-					_, dup = readErrnoSeen.LoadOrStore(errno, true)
-				}
-				if !dup {
-					t.Logf("ephemeral error: %v", err)
-				}
-				return
-			} else {
-				t.Errorf("unexpected error: %v", err)
-				return
-			}
-
-			if len(data) != 8*chunkWords {
-				t.Errorf("read %d bytes, but each write is a %d-byte file", len(data), 8*chunkWords)
-				return
-			}
-
-			u := binary.LittleEndian.Uint64(data)
-			for i := 1; i < chunkWords; i++ {
-				next := binary.LittleEndian.Uint64(data[i*8:])
-				if next != u+1 {
-					t.Errorf("wrote sequential integers, but read integer out of sequence at offset %d", i)
-					return
-				}
-				u = next
-			}
-		}()
-	}
-
-	for n := parallel; n > 0; n-- {
-		sem <- true
-	}
-
-	var minWriteSuccesses int64 = attempts
-	if runtime.GOOS == "windows" {
-		// Windows produces frequent "Access is denied" errors under heavy rename load.
-		// As long as those are the only errors and *some* of the writes succeed, we're happy.
-		minWriteSuccesses = attempts / 4
-	}
-
-	if writeSuccesses < minWriteSuccesses {
-		t.Errorf("%d (of %d) writes succeeded; want ≥ %d", writeSuccesses, attempts, minWriteSuccesses)
-	} else {
-		t.Logf("%d (of %d) writes succeeded (ok: ≥ %d)", writeSuccesses, attempts, minWriteSuccesses)
-	}
-
-	var minReadSuccesses int64 = attempts
-
-	switch runtime.GOOS {
-	case "windows":
-		// Windows produces frequent "Access is denied" errors under heavy rename load.
-		// As long as those are the only errors and *some* of the reads succeed, we're happy.
-		minReadSuccesses = attempts / 4
-
-	case "darwin", "ios":
-		// The filesystem on certain versions of macOS (10.14) and iOS (affected
-		// versions TBD) occasionally fail with "no such file or directory" errors.
-		// See https://golang.org/issue/33041 and https://golang.org/issue/42066.
-		// The flake rate is fairly low, so ensure that at least 75% of attempts
-		// succeed.
-		minReadSuccesses = attempts - (attempts / 4)
-	}
-
-	if readSuccesses < minReadSuccesses {
-		t.Errorf("%d (of %d) reads succeeded; want ≥ %d", readSuccesses, attempts, minReadSuccesses)
-	} else {
-		t.Logf("%d (of %d) reads succeeded (ok: ≥ %d)", readSuccesses, attempts, minReadSuccesses)
-	}
-}

src/cmd/go/internal/renameio/umask_test.go

@@ -1,43 +0,0 @@
-// 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.
-
-//go:build !plan9 && !windows && !js
-// +build !plan9,!windows,!js
-
-package renameio
-
-import (
-	"io/fs"
-	"os"
-	"path/filepath"
-	"syscall"
-	"testing"
-)
-
-func TestWriteFileModeAppliesUmask(t *testing.T) {
-	dir, err := os.MkdirTemp("", "renameio")
-	if err != nil {
-		t.Fatalf("Failed to create temporary directory: %v", err)
-	}
-	defer os.RemoveAll(dir)
-
-	const mode = 0644
-	const umask = 0007
-	defer syscall.Umask(syscall.Umask(umask))
-
-	file := filepath.Join(dir, "testWrite")
-	err = WriteFile(file, []byte("go-build"), mode)
-	if err != nil {
-		t.Fatalf("Failed to write file: %v", err)
-	}
-
-	fi, err := os.Stat(file)
-	if err != nil {
-		t.Fatalf("Stat %q (looking for mode %#o): %s", file, mode, err)
-	}
-
-	if fi.Mode()&fs.ModePerm != 0640 {
-		t.Errorf("Stat %q: mode %#o want %#o", file, fi.Mode()&fs.ModePerm, 0640)
-	}
-}