-
Notifications
You must be signed in to change notification settings - Fork 374
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
} | ||
close := func() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't quite see why There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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() } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.