remove isReallyNil() #3381
remove isReallyNil() #3381
Conversation
krnowak
left a comment
There was a problem hiding this comment.
I wrote this code long time ago, so I may be wrong with my review.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer func() { maybeClose(roc) }() |
There was a problem hiding this comment.
Does that cause roc to be "leaked" when if url.Parse(location) fails?
| if err != nil { | ||
| return nil, errwrap.Wrap(errors.New("error opening ACI file"), err) | ||
| } | ||
| defer func() { maybeClose(aciFile) }() |
There was a problem hiding this comment.
Does that cause the aciFile to "leak" if newValidator fails?
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| defer func() { maybeClose(aciFile) }() |
There was a problem hiding this comment.
Does this cause aciFile to "leak" if cd.UseCached is true?
rkt/image/httpops.go
Outdated
| session := o.getSession(u, aciFile.File, "ACI", etag) | ||
| dl := o.getDownloader(session) | ||
| if err := dl.Download(u, aciFile.File); err != nil { | ||
| aciFile.Close() // download failed, close the aciFile to implicitely release the lock |
There was a problem hiding this comment.
Aha, there you can see why I added this maybeClose function… To avoid closing the file in every error case. Instead I preferred the deffered maybeClose function call that does nothing if aciFile became nil (which would happen if no errors happened).
There was a problem hiding this comment.
Also, a typo - should be implicitly.
There was a problem hiding this comment.
Agreed, this is indeed the biggest benefit of maybeClose.
There was a problem hiding this comment.
And also agreed, that this PR introduces a stylistic nuance. Nevertheless I believe that (fortunately, or unfortunately) this is Go's nature, namely being pretty verbose, especially in error cases. The biggest pro of maybeClose is, that it reduces LOC, but it made it hard to reason about when these files were actually "really" closed. This will make the reasoning easier at the expense of verbosity.
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| defer func() { maybeClose(aciFile) }() |
There was a problem hiding this comment.
You are closing this if download fails, but you still leak it if cd.UseCached is true…
|
@krnowak thanks a lot for chiming in! o/ :-) I just took over, and you are very right in your assertion regarding the above leaks, thanks! |
55bcee4 to
ad0ca49
Compare
d6b7657 to
3904043
Compare
isReallyNil uses reflection to evaluate whether a given readSeekCloser is "really nil" to optionally close it. While it removes the burden of explicitly closing resources it is not obvious when it happens. This removes the function. Fixes rkt#2922
3904043 to
b9ffaa4
Compare
lucab
left a comment
There was a problem hiding this comment.
A couple of iterations offline, and now it's LGTM.
|
build failures are caused by a known flake |
This PR implements the NopCloser()-like function NopReadSeekCloser() and some other cleanups related to the removing of isReallyNil().
Fixes #2922
Supersedes #3120