pkg/download: close files after extracting in tar #310
Conversation
- Close file handle after extracting files on windows in extractTARGZ(). - Also ensure we close when io.Copy fails on extractZIP(). - Log the ignored error from the cleanup of DownloadPath. Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
| @@ -131,9 +132,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.
Would a defer f.Close() not be a cleaner solution?
There was a problem hiding this comment.
we could end up with have thousands of files open and end up exceeding open fd limit. no need to delay.
| @@ -74,15 +74,16 @@ func extractZIP(targetDir string, read io.ReaderAt, size int64) error { | |||
| if err != nil { | |||
| return errors.Wrap(err, "can't create file in zip destination dir") | |||
There was a problem hiding this comment.
I guess we should src.Close() here?
There was a problem hiding this comment.
yeah I think this is valid.
| @@ -74,15 +74,16 @@ func extractZIP(targetDir string, read io.ReaderAt, size int64) error { | |||
| if err != nil { | |||
| return errors.Wrap(err, "can't create file in zip destination dir") | |||
| } | |||
| close := func() { | |||
There was a problem hiding this comment.
I don't quite see why defer can't be used here.
There was a problem hiding this comment.
it would defer until all of the files are extracted.
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Codecov Report
@@ Coverage Diff @@
## master #310 +/- ##
==========================================
+ Coverage 55.66% 55.72% +0.05%
==========================================
Files 19 19
Lines 918 926 +8
==========================================
+ Hits 511 516 +5
- Misses 355 358 +3
Partials 52 52
Continue to review full report at Codecov.
|
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, corneliusweig The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #308.