From 9e34c791c9e00ab0c8af49ebd17f259fb0081cdf Mon Sep 17 00:00:00 2001 From: Alex Duchesne Date: Mon, 11 Oct 2021 20:15:38 -0400 Subject: [PATCH 1/3] Better temp file cleanup on Windows. --- internal/fs/file_windows.go | 53 +++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/internal/fs/file_windows.go b/internal/fs/file_windows.go index 8a4d01fb0..423492fa5 100644 --- a/internal/fs/file_windows.go +++ b/internal/fs/file_windows.go @@ -1,10 +1,41 @@ package fs import ( - "io/ioutil" "os" "path/filepath" "strings" + "syscall" + "strconv" + "sync" + "time" +) + +// Random number state. +// We generate random temporary file names so that there's a good +// chance the file doesn't exist yet - keeps the number of tries in +// TempFile to a minimum. +var rand uint32 +var randmu sync.Mutex + +func reseed() uint32 { + return uint32(time.Now().UnixNano() + int64(os.Getpid())) +} + +func nextRandom() string { + randmu.Lock() + r := rand + if r == 0 { + r = reseed() + } + r = r*1664525 + 1013904223 // constants from Numerical Recipes + rand = r + randmu.Unlock() + return strconv.Itoa(int(1e9 + r%1e9))[1:] +} + +const ( + FILE_ATTRIBUTE_TEMPORARY = 0x00000100 + FILE_FLAG_DELETE_ON_CLOSE = 0x04000000 ) // fixpath returns an absolute path on windows, so restic can open long file @@ -32,7 +63,25 @@ func fixpath(name string) string { // TempFile creates a temporary file. func TempFile(dir, prefix string) (f *os.File, err error) { - return ioutil.TempFile(dir, prefix) + if dir == "" { + dir = os.TempDir() + } + + access := uint32(syscall.GENERIC_READ | syscall.GENERIC_WRITE) + creation := uint32(syscall.CREATE_NEW) + flags := uint32(FILE_ATTRIBUTE_TEMPORARY | FILE_FLAG_DELETE_ON_CLOSE) + + for i := 0; i < 10000; i++ { + path := filepath.Join(dir, prefix+nextRandom()) + + h, err := syscall.CreateFile(syscall.StringToUTF16Ptr(path), access, 0, nil, creation, flags, 0) + if err == nil { + return os.NewFile(uintptr(h), path), nil + } + } + + // Proper error handling is still to do + return nil, os.ErrExist } // Chmod changes the mode of the named file to mode. From 9a3f1a970394d8e69f7321ca0159002c6725754e Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Wed, 29 Dec 2021 22:07:17 +0100 Subject: [PATCH 2/3] Simplify and comment TempFile implementation for windows --- changelog/unreleased/issue-3465 | 10 +++++ internal/fs/file_windows.go | 71 ++++++++++++++------------------- 2 files changed, 40 insertions(+), 41 deletions(-) create mode 100644 changelog/unreleased/issue-3465 diff --git a/changelog/unreleased/issue-3465 b/changelog/unreleased/issue-3465 new file mode 100644 index 000000000..b108461d4 --- /dev/null +++ b/changelog/unreleased/issue-3465 @@ -0,0 +1,10 @@ +Enhancement: Improve handling of temporary files on Windows + +In some cases restic failed to delete temporary files causing the current +command to fail. This has been fixed by ensuring that Windows automatically +deletes the file. In addition, temporary files are only written to disk if +necessary to reduce disk writes. + +https://github.com/restic/restic/issues/3465 +https://github.com/restic/restic/issues/1551 +https://github.com/restic/restic/pull/3610 diff --git a/internal/fs/file_windows.go b/internal/fs/file_windows.go index 423492fa5..56effdc18 100644 --- a/internal/fs/file_windows.go +++ b/internal/fs/file_windows.go @@ -1,41 +1,14 @@ package fs import ( + "math/rand" "os" "path/filepath" - "strings" - "syscall" "strconv" - "sync" + "strings" "time" -) -// Random number state. -// We generate random temporary file names so that there's a good -// chance the file doesn't exist yet - keeps the number of tries in -// TempFile to a minimum. -var rand uint32 -var randmu sync.Mutex - -func reseed() uint32 { - return uint32(time.Now().UnixNano() + int64(os.Getpid())) -} - -func nextRandom() string { - randmu.Lock() - r := rand - if r == 0 { - r = reseed() - } - r = r*1664525 + 1013904223 // constants from Numerical Recipes - rand = r - randmu.Unlock() - return strconv.Itoa(int(1e9 + r%1e9))[1:] -} - -const ( - FILE_ATTRIBUTE_TEMPORARY = 0x00000100 - FILE_FLAG_DELETE_ON_CLOSE = 0x04000000 + "golang.org/x/sys/windows" ) // fixpath returns an absolute path on windows, so restic can open long file @@ -61,25 +34,41 @@ func fixpath(name string) string { return name } -// TempFile creates a temporary file. +// TempFile creates a temporary file which is marked as delete-on-close func TempFile(dir, prefix string) (f *os.File, err error) { + // slightly modified implementation of ioutil.TempFile(dir, prefix) to allow us to add + // the FILE_ATTRIBUTE_TEMPORARY | FILE_FLAG_DELETE_ON_CLOSE flags. + // These provide two large benefits: + // FILE_ATTRIBUTE_TEMPORARY tells Windows to keep the file in memory only if possible + // which reduces the amount of unnecessary disk writes. + // FILE_FLAG_DELETE_ON_CLOSE instructs Windows to automatically delete the file once + // all file descriptors are closed. + if dir == "" { dir = os.TempDir() } - access := uint32(syscall.GENERIC_READ | syscall.GENERIC_WRITE) - creation := uint32(syscall.CREATE_NEW) - flags := uint32(FILE_ATTRIBUTE_TEMPORARY | FILE_FLAG_DELETE_ON_CLOSE) - - for i := 0; i < 10000; i++ { - path := filepath.Join(dir, prefix+nextRandom()) + access := uint32(windows.GENERIC_READ | windows.GENERIC_WRITE) + creation := uint32(windows.CREATE_NEW) + share := uint32(0) // prevent other processes from accessing the file + flags := uint32(windows.FILE_ATTRIBUTE_TEMPORARY | windows.FILE_FLAG_DELETE_ON_CLOSE) - h, err := syscall.CreateFile(syscall.StringToUTF16Ptr(path), access, 0, nil, creation, flags, 0) - if err == nil { - return os.NewFile(uintptr(h), path), nil + rnd := rand.New(rand.NewSource(time.Now().UnixNano())) + for i := 0; i < 10000; i++ { + randSuffix := strconv.Itoa(int(1e9 + rnd.Intn(1e9)%1e9))[1:] + path := filepath.Join(dir, prefix+randSuffix) + + ptr, err := windows.UTF16PtrFromString(path) + if err != nil { + return nil, err } + h, err := windows.CreateFile(ptr, access, share, nil, creation, flags, 0) + if os.IsExist(err) { + continue + } + return os.NewFile(uintptr(h), path), err } - + // Proper error handling is still to do return nil, os.ErrExist } From 4077a81b344fe63541b43aceb05b3dbed2eb1951 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Wed, 29 Dec 2021 22:19:58 +0100 Subject: [PATCH 3/3] Add simple test for fs.TempFile on windows --- internal/fs/file_windows_test.go | 35 ++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 internal/fs/file_windows_test.go diff --git a/internal/fs/file_windows_test.go b/internal/fs/file_windows_test.go new file mode 100644 index 000000000..71077709b --- /dev/null +++ b/internal/fs/file_windows_test.go @@ -0,0 +1,35 @@ +package fs_test + +import ( + "errors" + "os" + "testing" + + "github.com/restic/restic/internal/fs" + rtest "github.com/restic/restic/internal/test" +) + +func TestTempFile(t *testing.T) { + // create two temp files at the same time to check that the + // collision avoidance works + f, err := fs.TempFile("", "test") + fn := f.Name() + rtest.OK(t, err) + f2, err := fs.TempFile("", "test") + fn2 := f2.Name() + rtest.OK(t, err) + rtest.Assert(t, fn != fn2, "filenames don't differ %s", fn) + + _, err = os.Stat(fn) + rtest.OK(t, err) + _, err = os.Stat(fn2) + rtest.OK(t, err) + + rtest.OK(t, f.Close()) + rtest.OK(t, f2.Close()) + + _, err = os.Stat(fn) + rtest.Assert(t, errors.Is(err, os.ErrNotExist), "err %s", err) + _, err = os.Stat(fn2) + rtest.Assert(t, errors.Is(err, os.ErrNotExist), "err %s", err) +}