Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Windows doesn't allow renaming files that are open (attempt 2) #171

Merged
merged 4 commits into from
Aug 25, 2017
Merged

Windows doesn't allow renaming files that are open (attempt 2) #171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
642155e
Merge pull request #7 from cloudfoundry/develop
Fydon Mar 6, 2017
fbf3bc7
Merge pull request #8 from cloudfoundry/develop
Fydon Mar 10, 2017
933b483
Retry process makes 3 attempts, so use 3 temp files. Also join path w…
Fydon Feb 17, 2017
f78baee
Ensure file is closed before moving
Fydon Feb 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions installation/tarball/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ func (p *provider) downloadRetryable(source Source) boshretry.Retryable {
return true, bosherr.WrapError(err, "Verifying digest for downloaded file")
}

downloadedFile.Close()

err = p.cache.Save(downloadedFile.Name(), source)
if err != nil {
return true, bosherr.WrapError(err, "Saving downloaded file in cache")
Expand Down
33 changes: 25 additions & 8 deletions installation/tarball/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
fakebiui "github.com/cloudfoundry/bosh-cli/ui/fakes"
fakebihttpclient "github.com/cloudfoundry/bosh-utils/httpclient/fakes"
boshlog "github.com/cloudfoundry/bosh-utils/logger"
boshsys "github.com/cloudfoundry/bosh-utils/system"
fakesys "github.com/cloudfoundry/bosh-utils/system/fakes"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -77,18 +78,28 @@ var _ = Describe("Provider", func() {

Context("when tarball is not present in cache", func() {
var (
tempDownloadFilePath string
tempDownloadFilePath1 string
tempDownloadFilePath2 string
tempDownloadFilePath3 string
)

BeforeEach(func() {
tempDownloadFile, err := ioutil.TempFile("", "temp-download-file")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this test fail on windows if not changed to have 3 temp files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails everywhere now without 3 temp files, as the file is being closed on each retry. The real file system is creating a new temp file on each of the 3 retries, but the test was previously only using one temp file for all retries.

tempDownloadFile1, err := ioutil.TempFile("", "temp-download-file1")
Expect(err).ToNot(HaveOccurred())
fs.ReturnTempFile = tempDownloadFile
tempDownloadFilePath = tempDownloadFile.Name()
tempDownloadFile2, err := ioutil.TempFile("", "temp-download-file2")
Expect(err).ToNot(HaveOccurred())
tempDownloadFile3, err := ioutil.TempFile("", "temp-download-file3")
Expect(err).ToNot(HaveOccurred())
fs.ReturnTempFiles = []boshsys.File{tempDownloadFile1, tempDownloadFile2, tempDownloadFile3}
tempDownloadFilePath1 = tempDownloadFile1.Name()
tempDownloadFilePath2 = tempDownloadFile2.Name()
tempDownloadFilePath3 = tempDownloadFile3.Name()
})

AfterEach(func() {
os.RemoveAll(tempDownloadFilePath)
os.RemoveAll(tempDownloadFilePath1)
os.RemoveAll(tempDownloadFilePath2)
os.RemoveAll(tempDownloadFilePath3)
})

Context("when downloading succeds", func() {
Expand Down Expand Up @@ -137,7 +148,9 @@ var _ = Describe("Provider", func() {
It("removes the downloaded file", func() {
_, err := provider.Get(source, fakeStage)
Expect(err).To(HaveOccurred())
Expect(fs.FileExists(tempDownloadFilePath)).To(BeFalse())
Expect(fs.FileExists(tempDownloadFilePath1)).To(BeFalse())
Expect(fs.FileExists(tempDownloadFilePath2)).To(BeFalse())
Expect(fs.FileExists(tempDownloadFilePath3)).To(BeFalse())
})
})

Expand All @@ -156,7 +169,9 @@ var _ = Describe("Provider", func() {
It("removes the downloaded file", func() {
_, err := provider.Get(source, fakeStage)
Expect(err).To(HaveOccurred())
Expect(fs.FileExists(tempDownloadFilePath)).To(BeFalse())
Expect(fs.FileExists(tempDownloadFilePath1)).To(BeFalse())
Expect(fs.FileExists(tempDownloadFilePath2)).To(BeFalse())
Expect(fs.FileExists(tempDownloadFilePath3)).To(BeFalse())
})
})
})
Expand All @@ -179,7 +194,9 @@ var _ = Describe("Provider", func() {
It("removes the downloaded file", func() {
_, err := provider.Get(source, fakeStage)
Expect(err).To(HaveOccurred())
Expect(fs.FileExists(tempDownloadFilePath)).To(BeFalse())
Expect(fs.FileExists(tempDownloadFilePath1)).To(BeFalse())
Expect(fs.FileExists(tempDownloadFilePath2)).To(BeFalse())
Expect(fs.FileExists(tempDownloadFilePath3)).To(BeFalse())
})
})
})
Expand Down
2 changes: 2 additions & 0 deletions releasedir/fs_blobs_dir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,8 @@ func (d FSBlobsDir) TrackBlob(path string, src io.ReadCloser) (Blob, error) {

blobs[idx] = Blob{Path: path, Size: fileInfo.Size(), SHA1: sha1}

tempFile.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance for a test for this one? nothing currently fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but what should that test do? This method does defer the close of the temp file, but now the defer close will fail if this line is reached. The defer close isn't checking for failure, so this close is causing a silent failure, but also preventing an error in Windows environments.


err = d.moveBlobLocally(tempFile.Name(), filepath.Join(d.dirPath, path))
if err != nil {
return Blob{}, err
Expand Down