Fix handle leak #4567
Fix handle leak #4567
Conversation
Signed-off-by: Марк Булатов <markus7bulatenko@yandex.ru>
There was a problem hiding this comment.
Why not use defer?
func (hrs *httpReadSeeker) reader() (io.Reader, error) {
// ...
if resp.StatusCode >= 200 && resp.StatusCode <= 399 {
if hrs.readerOffset > 0 {
if resp.StatusCode != http.StatusPartialContent {
defer resp.Body.Close()
return nil, ErrWrongCodeForByteRange
}
contentRange := resp.Header.Get("Content-Range")
if contentRange == "" {
defer resp.Body.Close()
return nil, errors.New("no Content-Range header found in HTTP 206 response")
}
submatches := contentRangeRegexp.FindStringSubmatch(contentRange)
if len(submatches) < 4 {
defer resp.Body.Close()
return nil, fmt.Errorf("could not parse Content-Range header: %s", contentRange)
}
startByte, err := strconv.ParseUint(submatches[1], 10, 64)
if err != nil {
defer resp.Body.Close()
return nil, fmt.Errorf("could not parse start of range in Content-Range header: %s", contentRange)
}
if startByte != uint64(hrs.readerOffset) {
defer resp.Body.Close()
return nil, fmt.Errorf("received Content-Range starting at offset %d instead of requested %d", startByte, hrs.readerOffset)
}
endByte, err := strconv.ParseUint(submatches[2], 10, 64)
if err != nil {
defer resp.Body.Close()
return nil, fmt.Errorf("could not parse end of range in Content-Range header: %s", contentRange)
}
if submatches[3] == "*" {
hrs.size = -1
} else {
size, err := strconv.ParseUint(submatches[3], 10, 64)
if err != nil {
defer resp.Body.Close()
return nil, fmt.Errorf("could not parse total size in Content-Range header: %s", contentRange)
}
if endByte+1 != size {
defer resp.Body.Close()
return nil, fmt.Errorf("range in Content-Range stops before the end of the content: %s", contentRange)
}
hrs.size = int64(size)
}
} else if resp.StatusCode == http.StatusOK {
hrs.size = resp.ContentLength
} else {
hrs.size = -1
}
hrs.rc = resp.Body
} else {
defer resp.Body.Close()
if hrs.errorHandler != nil {
return nil, hrs.errorHandler(resp)
}
return nil, fmt.Errorf("unexpected status resolving reader: %v", resp.Status)
}
return hrs.rc, nil
}There was a problem hiding this comment.
If we use a named return for the error, we can probably handle this with a single defer; from a quick glance over this function, it looks like we want to close the response body if an error is returned; in other cases, closing the body must be handled by the caller;
So if we change the signature to have a named return for the error (we don't need a name for the io.Reader); generally I prefer using a distinctive name for such error returns (instead of err) to prevent accidental shadowing;
- func (hrs *httpReadSeeker) reader() (io.Reader, error) {
+ func (hrs *httpReadSeeker) reader() (_ io.Reader, retErr error) {Then we can define a defer after a non-error request was made;
resp, err := hrs.client.Do(req)
if err != nil {
return nil, err
}
defer func() {
if retErr != nil {
_ = resp.Body.Close()
}
}()This means (AFAICS) that we can also remove the defer resp.Body.Close() at the end of the function, although I'm not 100% sure if that hrs.errorHandler(resp) could potentially return a nil error? It looks like NewHTTPReadSeeker is the only place where this errorHandler is set, and it's not used in our codebase, but (technically) it could be "anything", so perhaps we could keep it.
if hrs.errorHandler != nil {
defer resp.Body.Close()
return nil, hrs.errorHandler(resp)
}Quick scratch I made locally with those changes applied looks like below (click "details" to expand);
func (hrs *httpReadSeeker) reader() (_ io.Reader, retErr error) {
if hrs.err != nil {
return nil, hrs.err
}
if hrs.rc != nil {
return hrs.rc, nil
}
req, err := http.NewRequest("GET", hrs.url, nil)
if err != nil {
return nil, err
}
if hrs.readerOffset > 0 {
// If we are at different offset, issue a range request from there.
req.Header.Add("Range", fmt.Sprintf("bytes=%d-", hrs.readerOffset))
// TODO: get context in here
// context.GetLogger(hrs.context).Infof("Range: %s", req.Header.Get("Range"))
}
resp, err := hrs.client.Do(req)
if err != nil {
return nil, err
}
defer func() {
if retErr != nil {
_ = resp.Body.Close()
}
}()
// Normally would use client.SuccessStatus, but that would be a cyclic
// import
if resp.StatusCode >= 200 && resp.StatusCode <= 399 {
if hrs.readerOffset > 0 {
if resp.StatusCode != http.StatusPartialContent {
return nil, ErrWrongCodeForByteRange
}
contentRange := resp.Header.Get("Content-Range")
if contentRange == "" {
return nil, errors.New("no Content-Range header found in HTTP 206 response")
}
submatches := contentRangeRegexp.FindStringSubmatch(contentRange)
if len(submatches) < 4 {
return nil, fmt.Errorf("could not parse Content-Range header: %s", contentRange)
}
startByte, err := strconv.ParseUint(submatches[1], 10, 64)
if err != nil {
return nil, fmt.Errorf("could not parse start of range in Content-Range header: %s", contentRange)
}
if startByte != uint64(hrs.readerOffset) {
return nil, fmt.Errorf("received Content-Range starting at offset %d instead of requested %d", startByte, hrs.readerOffset)
}
endByte, err := strconv.ParseUint(submatches[2], 10, 64)
if err != nil {
return nil, fmt.Errorf("could not parse end of range in Content-Range header: %s", contentRange)
}
if submatches[3] == "*" {
hrs.size = -1
} else {
size, err := strconv.ParseUint(submatches[3], 10, 64)
if err != nil {
return nil, fmt.Errorf("could not parse total size in Content-Range header: %s", contentRange)
}
if endByte+1 != size {
return nil, fmt.Errorf("range in Content-Range stops before the end of the content: %s", contentRange)
}
hrs.size = int64(size)
}
} else if resp.StatusCode == http.StatusOK {
hrs.size = resp.ContentLength
} else {
hrs.size = -1
}
hrs.rc = resp.Body
} else {
if hrs.errorHandler != nil {
// Closing the body should be handled by the existing defer,
// but in case a custom "errHandler" is used that doesn't return
// an error, we close the body regardless.
defer resp.Body.Close()
return nil, hrs.errorHandler(resp)
}
return nil, fmt.Errorf("unexpected status resolving reader: %v", resp.Status)
}
return hrs.rc, nil
}| @@ -190,30 +190,36 @@ func (hrs *httpReadSeeker) reader() (io.Reader, error) { | |||
| if resp.StatusCode >= 200 && resp.StatusCode <= 399 { | |||
| if hrs.readerOffset > 0 { | |||
| if resp.StatusCode != http.StatusPartialContent { | |||
| resp.Close() | |||
There was a problem hiding this comment.
At least these should be resp.Body.Close(), because resp.Close is a boolean (not a func).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
Pushed my quick draft as a PR against your PR branch; if you merge (and squash), those change should show up here if you first push to update the PR; |
|
maybe we can get this even into v3??? @milosgajdos @stevvooe |
|
@Bbulatov do you want to merge @thaJeztah 's PR into your branch so we can continue with this PR? |
registry/client/transport: use defer to close response body
|
This needs a rebase now @Bbulatov |
Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
|
OMG, I tried fixing this using GitHub's UI tools and it completely messed things up . @Bbulatov please remove my latest commit (it was committed via the extremely retarded resolve conflicts UI tool provided by GitHub) and then rebase. Sorry about this 🙄 🤦♂️ |
@Vad1mo feel free to open a PR on main or maybe @Bbulatov should do; we won't be doing another 2.x release and I've just noticed that this PR has been opened against the 2.x branch...so yeah, if you want this, please open this against the |
|
@milosgajdos, ok I'll open up a new one |
resolves distribution#4494 replaces distribution#4567
resolves distribution#4494 replaces distribution#4567 Signed-off-by: Vadim Bauer <vb@container-registry.com>
resolves distribution#4494 replaces distribution#4567 Signed-off-by: Vadim Bauer <vb@container-registry.com>
#4494