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.

Sign up for GitHub

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

Jump to bottom

"Reduce Scope of Variables" second good example can leak an open file #19

Closed
abbot opened this issue Oct 11, 2019 · 1 comment · Fixed by #20
Closed

"Reduce Scope of Variables" second good example can leak an open file #19

abbot opened this issue Oct 11, 2019 · 1 comment · Fixed by #20

Comments

@abbot
Copy link

abbot commented Oct 11, 2019

Copying my comment from HN thread:

I find it amusing that the advice in the style guide gives a good example contradicting another good example, and contains a subtle bug.

In the "Reduce Scope of Variables", second good example leaks an open file when WriteString fails, because it doesn't follow the own advice of "Defer to Clean Up" if you are curious.

(Handling that properly with a defer is a bit more tricky - something like https://play.golang.org/p/l1PeWM3Tisg).
@abhinav
Copy link
Collaborator

abhinav commented Oct 11, 2019

Good catch, thanks! I don't think that example needs to reference file handles
at all. We'll fix it.

abhinav added a commit that referenced this issue Oct 11, 2019
@abhinav
Reduce Scope: Fix FD leak
d5047de 
As pointed out in #19, the second Good example for "Reduce Scope of
Variables" leaks an open file descriptor if `io.WriteString` fails.

Further, both sets of examples are manually closing files instead of
using `defer` as pointed out elsewhere in the guide.

As these examples just need a pair of functions, one returning just an
error, another returning a value and an error, I've switched them to
using `ioutil.ReadFile`, `ioutil.WriteFile`, and a plausible
`Config.Decode([]byte) error` method on a `cfg` variable assumed to be
in-scope.

Resolves #19
@abhinav abhinav mentioned this issue Oct 11, 2019
Merged
abhinav added a commit that referenced this issue Oct 11, 2019
@abhinav
Reduce Scope: Fix FD leak (#20)
3baa2bd 
As pointed out in #19, the second Good example for "Reduce Scope of
Variables" leaks an open file descriptor if `io.WriteString` fails.

Further, both sets of examples are manually closing files instead of
using `defer` as pointed out elsewhere in the guide.

As these examples just need a pair of functions, one returning just an
error, another returning a value and an error, I've switched them to
using `ioutil.ReadFile`, `ioutil.WriteFile`, and a plausible
`Config.Decode([]byte) error` method on a `cfg` variable assumed to be
in-scope.

Resolves #19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Reduce Scope: Fix FD leak uber-go/guide
2 participants
@abbot @abhinav