From 0c8edfc097a8364819649c9ba4df2221082a8cf8 Mon Sep 17 00:00:00 2001 From: Marat Radchenko Date: Fri, 25 Oct 2019 10:10:03 +0300 Subject: [PATCH] Stop replacing files in LFS storage when downloading them concurrently on Windows On Windows, there is no way to replace file atomically. Instead, MoveFileExA: 1. Calls CreateFileA(access=Delete, shareMode=Delete) 2. Calls SetRenameInformationFile to rename file 3. Calls CloseFile The problem is that if parallel process attempts to open destination file for reading between steps 2 and 3, it will try to do that without shareMode=Delete and will hit a SHARING_VIOLATION error. In practice, this race condition results in: Smudge error: Error opening media file.: open .git\lfs\objects\: The process cannot access the file because it is being used by another process. There are two solutions here: 1. Do not overwrite file if it already exists 2. Retry reading from file if got a SHARING_VIOLATION This commit implements option 1. Fixes #2825 (for Windows, other OSes are race-free already). Also see #3813 and #3826. --- tools/util_darwin.go | 4 +++ tools/util_generic.go | 4 +++ tools/util_linux.go | 4 +++ tools/util_windows.go | 24 ++++++++++++++++ tools/util_windows_test.go | 58 ++++++++++++++++++++++++++++++++++++++ tq/basic_download.go | 15 ++++++---- 6 files changed, 103 insertions(+), 6 deletions(-) diff --git a/tools/util_darwin.go b/tools/util_darwin.go index 782e8cc6..8b57cda1 100644 --- a/tools/util_darwin.go +++ b/tools/util_darwin.go @@ -141,3 +141,7 @@ func cloneFileSyscall(dst, src string) *CloneFileError { return nil } + +func TryRename(oldname, newname string) error { + return RenameFileCopyPermissions(oldname, newname) +} diff --git a/tools/util_generic.go b/tools/util_generic.go index 5e912210..fab7f772 100644 --- a/tools/util_generic.go +++ b/tools/util_generic.go @@ -21,3 +21,7 @@ func CloneFile(writer io.Writer, reader io.Reader) (bool, error) { func CloneFileByPath(_, _ string) (bool, error) { return false, nil } + +func TryRename(oldname, newname string) error { + return RenameFileCopyPermissions(oldname, newname) +} diff --git a/tools/util_linux.go b/tools/util_linux.go index 35f2d1b6..850e9e94 100644 --- a/tools/util_linux.go +++ b/tools/util_linux.go @@ -71,3 +71,7 @@ func CloneFileByPath(dst, src string) (bool, error) { return CloneFile(dstFile, srcFile) } + +func TryRename(oldname, newname string) error { + return RenameFileCopyPermissions(oldname, newname) +} diff --git a/tools/util_windows.go b/tools/util_windows.go index 5ef8adc3..6cd6a4e3 100644 --- a/tools/util_windows.go +++ b/tools/util_windows.go @@ -6,6 +6,7 @@ import ( "io" "io/ioutil" "os" + "path/filepath" "unsafe" "golang.org/x/sys/windows" @@ -156,3 +157,26 @@ func roundUp(value, base int64) int64 { return value - mod + base } + +// This is a simplified variant of fixLongPath from file_windows.go. Unfortunately, that function is not public +func toSafePath(path string) (*uint16, error) { + abspath, err := filepath.Abs(path) + if err != nil { + return nil, err + } + + return windows.UTF16PtrFromString(`\\?\` + abspath) +} + +// This is almost the same as os.Rename but doesn't overwrite destination if it already exists +func TryRename(oldname, newname string) error { + from, err := toSafePath(oldname) + if err != nil { + return err + } + to, err := toSafePath(newname) + if err != nil { + return err + } + return windows.MoveFileEx(from, to, 0) +} diff --git a/tools/util_windows_test.go b/tools/util_windows_test.go index 189df336..e506eacd 100644 --- a/tools/util_windows_test.go +++ b/tools/util_windows_test.go @@ -109,3 +109,61 @@ func fillFile(target *os.File, size int64) (hash string, err error) { return hex.EncodeToString(sha.Sum(nil)), nil } + +func TestTryRenameDestExists(t *testing.T) { + source, err := ioutil.TempFile("", "source") + assert.NoError(t, err) + assert.NoError(t, source.Close()) + defer os.Remove(source.Name()) + + sourceData := []byte("source") + assert.NoError(t, ioutil.WriteFile(source.Name(), sourceData, 0644)) + + dest, err := ioutil.TempFile("", "dest") + assert.NoError(t, err) + defer os.Remove(dest.Name()) + assert.NoError(t, dest.Close()) + + destData := []byte("dest") + assert.NoError(t, ioutil.WriteFile(dest.Name(), destData, 0644)) + + // Perform rename + err = TryRename(source.Name(), dest.Name()) + assert.True(t, os.IsExist(err)) + + sourceData2, err := ioutil.ReadFile(source.Name()) + assert.NoError(t, err) + assert.Equal(t, sourceData, sourceData2) + + destData2, err := ioutil.ReadFile(dest.Name()) + assert.NoError(t, err) + assert.Equal(t, destData, destData2) +} + +func TestTryRename(t *testing.T) { + source, err := ioutil.TempFile("", "source") + assert.NoError(t, err) + assert.NoError(t, source.Close()) + defer os.Remove(source.Name()) + + sourceData := []byte("source") + assert.NoError(t, ioutil.WriteFile(source.Name(), sourceData, 0644)) + + dest, err := ioutil.TempFile("", "dest") + assert.NoError(t, err) + defer os.Remove(dest.Name()) + assert.NoError(t, dest.Close()) + + // Remove destination file + assert.NoError(t, os.Remove(dest.Name())) + + // Perform rename + assert.NoError(t, TryRename(source.Name(), dest.Name())) + + _, err = os.Stat(source.Name()) + assert.True(t, os.IsNotExist(err)) + + destData, err := ioutil.ReadFile(dest.Name()) + assert.NoError(t, err) + assert.Equal(t, sourceData, destData) +} diff --git a/tq/basic_download.go b/tq/basic_download.go index 0b21559e..a9fc96bd 100644 --- a/tq/basic_download.go +++ b/tq/basic_download.go @@ -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.TryRename(f.Name(), a.downloadFilename(t)) } return err @@ -254,12 +254,15 @@ func (a *basicDownloadAdapter) download(t *Transfer, cb ProgressCallback, authOk return fmt.Errorf("can't close tempfile %q: %v", dlfilename, err) } - err = tools.RenameFileCopyPermissions(dlfilename, t.Path) - if _, err2 := os.Stat(t.Path); err2 == nil { - // Target file already exists, possibly was downloaded by other git-lfs process - return nil + err = tools.TryRename(dlfilename, t.Path) + // If rename failed because file already exists, we do not treat it as error + // This can happen when multiple git-lfs processes are fetching files concurrently on Windows + // Note that dlfilename needs to be handled regardless of TryRename success + if err != nil && !os.IsExist(err) { + return err } - return err + + return nil } func configureBasicDownloadAdapter(m *Manifest) {