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

pkg/download: close files after extracting in tar #310

Merged
merged 2 commits into from
Aug 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 10 additions & 5 deletions pkg/download/downloader.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,19 @@ func extractZIP(targetDir string, read io.ReaderAt, size int64) error {

dst, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, f.Mode())
if err != nil {
src.Close()
return errors.Wrap(err, "can't create file in zip destination dir")
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should src.Close() here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I think this is valid.

}
close := func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite see why defer can't be used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

it would defer until all of the files are extracted.

src.Close()
dst.Close()
}

if _, err := io.Copy(dst, src); err != nil {
close()
return errors.Wrap(err, "can't copy content to zip destination file")
}

// Cleanup the open fd. Don't use defer in case of many files.
// Don't be blocking
src.Close()
dst.Close()
close()
}

return nil
Expand Down Expand Up @@ -131,9 +133,12 @@ func extractTARGZ(targetDir string, at io.ReaderAt, size int64) error {
if err != nil {
return errors.Wrapf(err, "failed to create file %q", path)
}
close := func() { f.Close() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a defer f.Close() not be a cleaner solution?

Copy link
Member Author

Choose a reason for hiding this comment

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

we could end up with have thousands of files open and end up exceeding open fd limit. no need to delay.

if _, err := io.Copy(f, tr); err != nil {
close()
return errors.Wrapf(err, "failed to copy %q from tar into file", hdr.Name)
}
close()
default:
return errors.Errorf("unable to handle file type %d for %q in tar", hdr.Typeflag, hdr.Name)
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/installation/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@ func install(op installOperation, opts InstallOpts) error {
}
defer func() {
glog.V(3).Infof("Deleting the download staging directory %s", op.downloadStagingDir)
_ = os.RemoveAll(op.downloadStagingDir)
if err := os.RemoveAll(op.downloadStagingDir); err != nil {
glog.Warningf("failed to clean up download staging directory: %s", err)
}
}()
if err := downloadAndExtract(op.downloadStagingDir, op.platform.URI, op.platform.Sha256, opts.ArchiveFileOverride); err != nil {
return errors.Wrap(err, "failed to download and extract")
Expand Down