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.

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

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

Merged
merged 4 commits into from
Aug 25, 2017

Conversation

Fydon
Copy link
Contributor

@Fydon Fydon commented Mar 12, 2017

This is a replacement for #121. All the tests now pass and the changes have been rebased to a recent version of the develop branch.

@cfdreddbot
Copy link

Hey Fydon!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@cppforlife
Copy link
Contributor

@Fydon will pull it in.

)

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.

@@ -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.

@Fydon
Copy link
Contributor Author

Fydon commented May 31, 2017

@cppforlife Should there be a defer close in provider.go to ensure that downloadedFile is always closed? In its current state could there be a resource leak?

Should I make defer close execute where I've added close? It seems that I could do this by splitting part of the function into another function or using embedded functions. The reason I ask this is that it might be confusing to have two close calls in the same function for the same resource, i.e. defer close and close.

@dpb587-pivotal dpb587-pivotal merged commit f78baee into cloudfoundry:develop Aug 25, 2017
@Fydon Fydon deleted the feature/AddWindowsSupport3 branch September 19, 2017 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants