Skip to content

add f.Close() after dockerignore.ReadAll(f) before return err#33860

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
lixiaobing1:masterFclose
Jun 29, 2017
Merged

add f.Close() after dockerignore.ReadAll(f) before return err#33860
cpuguy83 merged 1 commit intomoby:masterfrom
lixiaobing1:masterFclose

Conversation

@lixiaobing1
Copy link
Copy Markdown
Contributor

@lixiaobing1 lixiaobing1 commented Jun 28, 2017 •
edited
Loading

add f.Close() after dockerignore.ReadAll(f) before return err.

thaJeztah
thaJeztah requested changes Jun 28, 2017
View reviewed changes
builder/remotecontext/detect.go Outdated
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jun 28, 2017

Choose a reason for hiding this comment

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

Not sure this will work; f will no longer be closed before it's being removed in the filesToRemove loop below

Perhaps add f.Close() inside the if err != nil { instead?

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jun 28, 2017

Choose a reason for hiding this comment

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

Looks like that's happening indeed;

06:41:21 time="2017-06-28T06:41:21Z" level=error msg="failed to remove .dockerignore: remove C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\builder-dockerignore-process-test250723583\\.dockerignore: The process cannot access the file because it is being used by another process." 
06:41:21 2017/06/28 06:41:21 Directory should contain exactly 1 file(s), got 2

@thaJeztah thaJeztah added status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Jun 28, 2017
boaz0
boaz0 approved these changes Jun 28, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

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

LGTM but I prefer to have f.Close() before return err. Read more about why here: https://lk4d4.darth.io/posts/defer/

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 28, 2017

@ripcurld0 the current (defer) approach isn't working; see my comment above; #33860 (comment) (it's why CI is failing)

@lixiaobing1
Copy link
Copy Markdown
Contributor Author

lixiaobing1 commented Jun 28, 2017

ok, has put f.Close() before return err.

@boaz0
Copy link
Copy Markdown
Contributor

boaz0 commented Jun 28, 2017

@thaJeztah thanks. I did miss it.

thaJeztah
thaJeztah approved these changes Jun 28, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 28, 2017
vdemeester
vdemeester approved these changes Jun 28, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

@cyphar
Copy link
Copy Markdown
Contributor

cyphar commented Jun 28, 2017 •
edited
Loading

@ripcurld0 FYI, I think @LK4D4's point was that in small functions you don't want to use defer when it's trivial to see that there's no missing error cases. However, defer has a very unique benefit: a function's defers will be executed when unwinding a function call stack during a panic. So if a calling function is using recover() then only defer can guarantee things will run.

@boaz0
Copy link
Copy Markdown
Contributor

boaz0 commented Jun 28, 2017

@cyphar I totally agree. I just think this is one of the cases I wouldn't use defer.

@nodakai
Copy link
Copy Markdown

nodakai commented Jun 28, 2017

So we just need a single f.Close() right after dockerignore.ReadAll(f) ?

(By the way the commit message is no longer in sync with the code itself.)

@thaJeztah thaJeztah mentioned this pull request Jun 28, 2017
Closed
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 28, 2017

So we just need a single f.Close() right after dockerignore.ReadAll(f) ?

Ah, yes, that should work obviously

By the way the commit message is no longer in sync with the code itself

Good catch: @lixiaobing1 can you update your PR with the above changes?

@lixiaobing1 lixiaobing1 changed the title add defer f.Close() add f.Close() after dockerignore.ReadAll(f) before return err Jun 29, 2017
@lixiaobing1
Copy link
Copy Markdown
Contributor Author

lixiaobing1 commented Jun 29, 2017

@thaJeztah ok, commit message has been updated.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 29, 2017

Looks like there's a gofmt issue;

00:57:51 These files are not properly gofmt'd:
00:57:51  - builder/remotecontext/detect.go
00:57:51 
00:57:51 Please reformat the above files using "gofmt -s -w" and commit the result.

@lixiaobing1
Copy link
Copy Markdown
Contributor Author

lixiaobing1 commented Jun 29, 2017

sorry, has commit again.

@lixiaobing1 @thaJeztah
add f.Close() after dockerignore.ReadAll(f) before return err
3072fce 
Signed-off-by: lixiaobing10051267 <li.xiaobing1@zte.com.cn>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@cpuguy83 cpuguy83 merged commit e066edc into moby:master Jun 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

+2 more reviewers

@vdemeester vdemeester vdemeester approved these changes

@boaz0 boaz0 boaz0 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@lixiaobing1 @thaJeztah @boaz0 @cyphar @nodakai @vdemeester @cpuguy83 @GordonTheTurtle