From 1e59e50ae48a9f02d00580f173f481e723888c88 Mon Sep 17 00:00:00 2001 From: David Morhovich and David Varvel Date: Fri, 20 Feb 2015 17:57:50 -0500 Subject: [PATCH 1/5] Windows likes its files closed before removing them. --- tar_transformer.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tar_transformer.go b/tar_transformer.go index 006dbb8..38d3b32 100644 --- a/tar_transformer.go +++ b/tar_transformer.go @@ -136,6 +136,11 @@ func transformZipToTar(path, destPath string) (int64, error) { return 0, err } + err = zr.Close() + if err != nil { + return 0, err + } + err = os.Remove(path) if err != nil { return 0, err From 505a1fdcc5af7823f20d7c87d9e4d1c833c59053 Mon Sep 17 00:00:00 2001 From: "David Morhovich, David Varvel and John Shahid" Date: Tue, 24 Feb 2015 17:05:39 -0500 Subject: [PATCH 2/5] Fix file moving to be safe on Windows. Fix file moving to be safe on Windows. A shim was added that calls into the Win32 API / MoveFileEx that prevents an error occurring on windows if the destination already exists. This shim might not be needed once os.Replace lands, possibly in Go 1.5. See: https://github.com/golang/go/issues/8914 [#88907658] --- cached_downloader.go | 2 +- file_cache.go | 2 +- replace.go | 9 +++++++++ replace_windows.go | 41 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 2 deletions(-) create mode 100644 replace.go create mode 100644 replace_windows.go diff --git a/cached_downloader.go b/cached_downloader.go index cd25860..840d53e 100644 --- a/cached_downloader.go +++ b/cached_downloader.go @@ -23,7 +23,7 @@ type CachedDownloader interface { } func NoopTransform(source, destination string) (int64, error) { - err := os.Rename(source, destination) + err := replace(source, destination) if err != nil { return 0, err } diff --git a/file_cache.go b/file_cache.go index f949ae0..f13b765 100644 --- a/file_cache.go +++ b/file_cache.go @@ -105,7 +105,7 @@ func (c *FileCache) Add(cacheKey string, sourcePath string, size int64, cachingI uniqueName := fmt.Sprintf("%s-%d-%d", cacheKey, time.Now().UnixNano(), c.seq) cachePath := filepath.Join(c.cachedPath, uniqueName) - err := os.Rename(sourcePath, cachePath) + err := replace(sourcePath, cachePath) if err != nil { return nil, err } diff --git a/replace.go b/replace.go new file mode 100644 index 0000000..b4973a9 --- /dev/null +++ b/replace.go @@ -0,0 +1,9 @@ +// +build !windows + +package cacheddownloader + +import "os" + +func replace(src, dst string) error { + return os.Rename(src, dst) +} diff --git a/replace_windows.go b/replace_windows.go new file mode 100644 index 0000000..6af311c --- /dev/null +++ b/replace_windows.go @@ -0,0 +1,41 @@ +package cacheddownloader + +import ( + "syscall" + "unsafe" +) + +func replace(src, dst string) error { + kernel32, err := syscall.LoadLibrary("kernel32.dll") + if err != nil { + return err + } + defer syscall.FreeLibrary(kernel32) + moveFileExUnicode, err := syscall.GetProcAddress(kernel32, "MoveFileExW") + if err != nil { + return err + } + + srcString, err := syscall.UTF16PtrFromString(src) + if err != nil { + return err + } + + dstString, err := syscall.UTF16PtrFromString(dst) + if err != nil { + return err + } + + srcPtr := uintptr(unsafe.Pointer(srcString)) + dstPtr := uintptr(unsafe.Pointer(dstString)) + + MOVEFILE_REPLACE_EXISTING := 0x1 + flag := uintptr(MOVEFILE_REPLACE_EXISTING) + + _, _, callErr := syscall.Syscall(uintptr(moveFileExUnicode), 3, srcPtr, dstPtr, flag) + if callErr != 0 { + return callErr + } + + return nil +} From d67cf5c323008fe69a1c1bc6f1540d8c97272192 Mon Sep 17 00:00:00 2001 From: David Varvel and John Shahid Date: Wed, 25 Feb 2015 15:39:13 -0500 Subject: [PATCH 3/5] Close the tar file that gets created on zip transformation. For windows compatibility. We added a test for windows that exercises this. --- file_cache.go | 2 +- replace.go | 2 + replace_windows.go | 4 ++ tar_transformer.go | 1 + tar_transformer_windows_test.go | 68 +++++++++++++++++++++++++++++++++ 5 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 tar_transformer_windows_test.go diff --git a/file_cache.go b/file_cache.go index f13b765..792e8cb 100644 --- a/file_cache.go +++ b/file_cache.go @@ -80,7 +80,7 @@ func (e *fileCacheEntry) readCloser() (*CachedFile, error) { return readCloser, nil } -func (c *FileCache) Add(cacheKey string, sourcePath string, size int64, cachingInfo CachingInfoType) (*CachedFile, error) { +func (c *FileCache) Add(cacheKey, sourcePath string, size int64, cachingInfo CachingInfoType) (*CachedFile, error) { lock.Lock() defer lock.Unlock() diff --git a/replace.go b/replace.go index b4973a9..f9fc404 100644 --- a/replace.go +++ b/replace.go @@ -4,6 +4,8 @@ package cacheddownloader import "os" +// if you are wondering why we have this function, see `replace' +// implementation in replace_windows.go func replace(src, dst string) error { return os.Rename(src, dst) } diff --git a/replace_windows.go b/replace_windows.go index 6af311c..38ccfe4 100644 --- a/replace_windows.go +++ b/replace_windows.go @@ -5,6 +5,10 @@ import ( "unsafe" ) +// Replaces `dst' with `src' atomically. Under linux we only have to +// call os.Rename(), on windows os.Rename() will error if the +// destination exists already. The replace function serves as a +// unified interface on both platforms. func replace(src, dst string) error { kernel32, err := syscall.LoadLibrary("kernel32.dll") if err != nil { diff --git a/tar_transformer.go b/tar_transformer.go index 38d3b32..30bbf47 100644 --- a/tar_transformer.go +++ b/tar_transformer.go @@ -110,6 +110,7 @@ func transformZipToTar(path, destPath string) (int64, error) { if err != nil { return 0, err } + defer dest.Close() zr, err := zip.OpenReader(path) if err != nil { diff --git a/tar_transformer_windows_test.go b/tar_transformer_windows_test.go new file mode 100644 index 0000000..537f08d --- /dev/null +++ b/tar_transformer_windows_test.go @@ -0,0 +1,68 @@ +package cacheddownloader_test + +import ( + "io/ioutil" + "os" + "path/filepath" + + "github.com/pivotal-golang/archiver/extractor/test_helper" + . "github.com/pivotal-golang/cacheddownloader" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("TarTransformer", func() { + var ( + scratch string + + sourcePath string + destinationPath string + + transformedSize int64 + transformErr error + ) + + archiveFiles := []test_helper.ArchiveFile{ + {Name: "some-file", Body: "some-contents"}, + } + + BeforeEach(func() { + var err error + + scratch, err = ioutil.TempDir("", "tar-transformer-scratch") + Expect(err).ShouldNot(HaveOccurred()) + + destinationFile, err := ioutil.TempFile("", "destination") + Expect(err).ShouldNot(HaveOccurred()) + + err = destinationFile.Close() + Expect(err).ShouldNot(HaveOccurred()) + + destinationPath = destinationFile.Name() + }) + + AfterEach(func() { + err := os.RemoveAll(scratch) + Expect(err).ShouldNot(HaveOccurred()) + }) + + JustBeforeEach(func() { + transformedSize, transformErr = TarTransform(sourcePath, destinationPath) + }) + + Context("when the file is a .zip", func() { + BeforeEach(func() { + sourcePath = filepath.Join(scratch, "file.zip") + + test_helper.CreateZipArchive(sourcePath, archiveFiles) + }) + + It("closes the tarfile", func() { + // On Windows, you can't remove files that are still open. On Linux, you can. + err := os.Remove(destinationPath) + + Expect(err).ShouldNot(HaveOccurred()) + }) + }) +}) From 6050dbbd8babdddc2a072851cc7d211265e47c53 Mon Sep 17 00:00:00 2001 From: Dave Goddard & John Shahid Date: Thu, 26 Feb 2015 10:20:51 -0500 Subject: [PATCH 4/5] Fix the last failing test on windows Windows timer increments in 15.6ms ticks, which caused the eviction test to fail since all files in the cache (sometimes) have the same timestamp. The patch adds a sleep before fetching `A' to make sure it has a more recent timestamp. --- cached_downloader_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cached_downloader_test.go b/cached_downloader_test.go index 351cd44..93ebd62 100644 --- a/cached_downloader_test.go +++ b/cached_downloader_test.go @@ -472,6 +472,9 @@ var _ = Describe("File cache", func() { )) url, _ = Url.Parse(server.URL() + "/A") + // windows timer increments every 15.6ms, without the sleep + // A, B & C will sometimes have the same timestamp + time.Sleep(16 * time.Microsecond) cache.Fetch(url, "A", NoopTransform, cancelChan) }) From 176d84dcb310c31ac8cffaeb49311ba5b8600533 Mon Sep 17 00:00:00 2001 From: "L. David Varvel" Date: Thu, 26 Feb 2015 13:52:40 -0500 Subject: [PATCH 5/5] Sleep for milliseconds rather than microseconds --- cached_downloader_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cached_downloader_test.go b/cached_downloader_test.go index 93ebd62..f9fa7a2 100644 --- a/cached_downloader_test.go +++ b/cached_downloader_test.go @@ -474,7 +474,7 @@ var _ = Describe("File cache", func() { url, _ = Url.Parse(server.URL() + "/A") // windows timer increments every 15.6ms, without the sleep // A, B & C will sometimes have the same timestamp - time.Sleep(16 * time.Microsecond) + time.Sleep(16 * time.Millisecond) cache.Fetch(url, "A", NoopTransform, cancelChan) })