Skip to content

Commit

Permalink
Implement retry logic to fix LFS storage race conditions on Windows
Browse files Browse the repository at this point in the history
Testing showed that while race condition analysis in #3880 was correct, the way it tries to fix that
does not work for the *first* git-lfs process that will actually perform file move.

Instead, this commit performs multiple attempts when working with files in LFS storage.

Similar logic is already implemented in "cmd/go/internal/robustio" and "cmd/go/internal/renameio" packages.
However, they are not public, so we cannot use them.
  • Loading branch information
slonopotamus committed Oct 29, 2019
1 parent 470398c commit 67c173b
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 4 deletions.
1 change: 1 addition & 0 deletions go.mod
Expand Up @@ -2,6 +2,7 @@ module github.com/git-lfs/git-lfs

require (
github.com/alexbrainman/sspi v0.0.0-20180125232955-4729b3d4d858
github.com/avast/retry-go v2.4.2+incompatible
github.com/git-lfs/gitobj v1.4.1
github.com/git-lfs/go-netrc v0.0.0-20180525200031-e0e9ca483a18
github.com/git-lfs/go-ntlm v0.0.0-20190401175752-c5056e7fa066
Expand Down
2 changes: 2 additions & 0 deletions go.sum
@@ -1,5 +1,7 @@
github.com/alexbrainman/sspi v0.0.0-20180125232955-4729b3d4d858 h1:OZQyEhf4BviydsRdmK4ryeJHotDLd7vL1X8+nnxXkfk=
github.com/alexbrainman/sspi v0.0.0-20180125232955-4729b3d4d858/go.mod h1:976q2ETgjT2snVCf2ZaBnyBbVoPERGjUz+0sofzEfro=
github.com/avast/retry-go v2.4.2+incompatible h1:+ZjCypQT/CyP0kyJO2EcU4d/ZEJWSbP8NENI578cPmA=
github.com/avast/retry-go v2.4.2+incompatible/go.mod h1:XtSnn+n/sHqQIpZ10K1qAevBhOOCWBLXXy3hyiqqBrY=
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/git-lfs/gitobj v1.4.1 h1:6nH5d1QP7GJjZfBqaBXpS7mDzT4plXQLqUjPbcbtRpw=
Expand Down
2 changes: 1 addition & 1 deletion lfs/gitfilter_smudge.go
Expand Up @@ -120,7 +120,7 @@ func (f *GitFilter) downloadFile(writer io.Writer, ptr *Pointer, workingfile, me
}

func (f *GitFilter) readLocalFile(writer io.Writer, ptr *Pointer, mediafile string, workingfile string, cb tools.CopyCallback) (int64, error) {
reader, err := os.Open(mediafile)
reader, err := tools.RobustOpen(mediafile)
if err != nil {
return 0, errors.Wrapf(err, "error opening media file")
}
Expand Down
2 changes: 1 addition & 1 deletion tools/filetools.go
Expand Up @@ -80,7 +80,7 @@ func RenameFileCopyPermissions(srcfile, destfile string) error {
}
}

if err := os.Rename(srcfile, destfile); err != nil {
if err := RobustRename(srcfile, destfile); err != nil {
return fmt.Errorf("cannot replace %q with %q: %v", destfile, srcfile, err)
}
return nil
Expand Down
13 changes: 13 additions & 0 deletions tools/robustio.go
@@ -0,0 +1,13 @@
// +build !windows

package tools

import "os"

func RobustRename(oldpath, newpath string) error {
return os.Rename(oldpath, newpath)
}

func RobustOpen(name string) (*os.File, error) {
return os.Open(name)
}
42 changes: 42 additions & 0 deletions tools/robustio_windows.go
@@ -0,0 +1,42 @@
// +build windows

package tools

import (
"github.com/avast/retry-go"
"os"
"syscall"
)

const (
// This is private in [go]/src/internal/syscall/windows/syscall_windows.go :(
ERROR_SHARING_VIOLATION syscall.Errno = 32
)

// isEphemeralError returns true if err may be resolved by waiting.
func isEphemeralError(err error) bool {
return err == ERROR_SHARING_VIOLATION
}

func RobustRename(oldpath, newpath string) error {
return retry.Do(
func() error {
return os.Rename(oldpath, newpath)
},
retry.RetryIf(isEphemeralError),
retry.LastErrorOnly(true),
)
}

func RobustOpen(name string) (*os.File, error) {
var result *os.File
return result, retry.Do(
func() error {
f, err := os.Open(name)
result = f
return err
},
retry.RetryIf(isEphemeralError),
retry.LastErrorOnly(true),
)
}
4 changes: 2 additions & 2 deletions tq/basic_download.go
Expand Up @@ -62,7 +62,7 @@ func (a *basicDownloadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb Progr
}

// Attempt to resume download. No error checking here. If we fail, we'll simply download from the start
os.Rename(a.downloadFilename(t), f.Name())
tools.RobustRename(a.downloadFilename(t), f.Name())

// Open temp file. It is either empty or partially downloaded
f, err = os.OpenFile(f.Name(), os.O_RDWR, 0644)
Expand Down Expand Up @@ -100,7 +100,7 @@ func (a *basicDownloadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb Progr
f.Close()
// Rename file so next download can resume from where we stopped.
// No error checking here, if rename fails then file will be deleted and there just will be no download resuming
os.Rename(f.Name(), a.downloadFilename(t))
tools.RobustRename(f.Name(), a.downloadFilename(t))
}

return err
Expand Down

0 comments on commit 67c173b

Please sign in to comment.