Skip to content

Commit 662a624

Browse files
slonopotamusslonopotamus
committed
Implement retry logic to fix LFS storage race conditions on Windows
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.
1 parent 285eebd commit 662a624

7 files changed

Lines changed: 79 additions & 4 deletions

File tree

‎go.mod‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ module github.com/git-lfs/git-lfs
22

33
require (
44
github.com/alexbrainman/sspi v0.0.0-20180125232955-4729b3d4d858
5+
github.com/avast/retry-go v2.4.2+incompatible
56
github.com/git-lfs/gitobj v1.4.1
67
github.com/git-lfs/go-netrc v0.0.0-20180525200031-e0e9ca483a18
78
github.com/git-lfs/go-ntlm v0.0.0-20190401175752-c5056e7fa066

‎go.sum‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
github.com/alexbrainman/sspi v0.0.0-20180125232955-4729b3d4d858 h1:OZQyEhf4BviydsRdmK4ryeJHotDLd7vL1X8+nnxXkfk=
22
github.com/alexbrainman/sspi v0.0.0-20180125232955-4729b3d4d858/go.mod h1:976q2ETgjT2snVCf2ZaBnyBbVoPERGjUz+0sofzEfro=
3+
github.com/avast/retry-go v2.4.2+incompatible h1:+ZjCypQT/CyP0kyJO2EcU4d/ZEJWSbP8NENI578cPmA=
4+
github.com/avast/retry-go v2.4.2+incompatible/go.mod h1:XtSnn+n/sHqQIpZ10K1qAevBhOOCWBLXXy3hyiqqBrY=
35
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
46
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
57
github.com/git-lfs/gitobj v1.4.1 h1:6nH5d1QP7GJjZfBqaBXpS7mDzT4plXQLqUjPbcbtRpw=

‎lfs/gitfilter_smudge.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ func (f *GitFilter) downloadFile(writer io.Writer, ptr *Pointer, workingfile, me
120120
}
121121

122122
func (f *GitFilter) readLocalFile(writer io.Writer, ptr *Pointer, mediafile string, workingfile string, cb tools.CopyCallback) (int64, error) {
123-
reader, err := os.Open(mediafile)
123+
reader, err := tools.RobustOpen(mediafile)
124124
if err != nil {
125125
return 0, errors.Wrapf(err, "error opening media file")
126126
}

‎tools/filetools.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ func RenameFileCopyPermissions(srcfile, destfile string) error {
8080
}
8181
}
8282

83-
if err := os.Rename(srcfile, destfile); err != nil {
83+
if err := RobustRename(srcfile, destfile); err != nil {
8484
return fmt.Errorf("cannot replace %q with %q: %v", destfile, srcfile, err)
8585
}
8686
return nil

‎tools/robustio.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// +build !windows
2+
3+
package tools
4+
5+
import "os"
6+
7+
func RobustRename(oldpath, newpath string) error {
8+
return os.Rename(oldpath, newpath)
9+
}
10+
11+
func RobustOpen(name string) (*os.File, error) {
12+
return os.Open(name)
13+
}

‎tools/robustio_windows.go‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
// +build windows
2+
3+
package tools
4+
5+
import (
6+
"os"
7+
"syscall"
8+
9+
"github.com/avast/retry-go"
10+
)
11+
12+
const (
13+
// This is private in [go]/src/internal/syscall/windows/syscall_windows.go :(
14+
ERROR_SHARING_VIOLATION syscall.Errno = 32
15+
)
16+
17+
func underlyingError(err error) error {
18+
switch err := err.(type) {
19+
case *os.PathError:
20+
return err.Err
21+
case *os.LinkError:
22+
return err.Err
23+
case *os.SyscallError:
24+
return err.Err
25+
}
26+
return err
27+
}
28+
29+
// isEphemeralError returns true if err may be resolved by waiting.
30+
func isEphemeralError(err error) bool {
31+
// TODO: Use this instead for Go >= 1.13
32+
// return errors.Is(err, ERROR_SHARING_VIOLATION)
33+
34+
err = underlyingError(err)
35+
return err == ERROR_SHARING_VIOLATION
36+
}
37+
38+
func RobustRename(oldpath, newpath string) error {
39+
return retry.Do(
40+
func() error {
41+
return os.Rename(oldpath, newpath)
42+
},
43+
retry.RetryIf(isEphemeralError),
44+
retry.LastErrorOnly(true),
45+
)
46+
}
47+
48+
func RobustOpen(name string) (*os.File, error) {
49+
var result *os.File
50+
return result, retry.Do(
51+
func() error {
52+
f, err := os.Open(name)
53+
result = f
54+
return err
55+
},
56+
retry.RetryIf(isEphemeralError),
57+
retry.LastErrorOnly(true),
58+
)
59+
}

‎tq/basic_download.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func (a *basicDownloadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb Progr
6262
}
6363

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

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

106106
return err

0 commit comments

Comments
 (0)