Skip to content

Commit

Permalink
httploader: Close resp body on bad status code
Browse files Browse the repository at this point in the history
Related to #5158
  • Loading branch information
mholt committed Oct 24, 2022
1 parent bbe3663 commit 817470d
Showing 1 changed file with 1 addition and 0 deletions.
1 change: 1 addition & 0 deletions caddyconfig/httploader.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ func attemptHttpCall(client *http.Client, request *http.Request) (*http.Response
if err != nil {
return nil, fmt.Errorf("problem calling http loader url: %v", err)
} else if resp.StatusCode < 200 || resp.StatusCode > 499 {
resp.Body.Close()

This comment has been minimized.

Copy link
@mohammed90

mohammed90 Oct 24, 2022

Member

Do we not need to drain the body?

This comment has been minimized.

Copy link
@mholt

mholt Oct 28, 2022

Author Member

Oh... maybe. That might affect connection reuse only. I don't know if that's still a thing 🤔

This comment has been minimized.

Copy link
@mohammed90

mohammed90 Oct 29, 2022

Member

I've just tried to trace the calls. We're constructing a new *http.Client every time, so re-use is not possible anwyays, so you're likely right. I think (not 100% sure) the fd will be returned to the OS on garbage collection.

N.B., through my tracing activity, I found resp.Body.Close() is potentially called multiple times on the final response body, because it's being Close()-ed here and in:

defer resp.Body.Close()

This comment has been minimized.

Copy link
@mholt

mholt Oct 29, 2022

Author Member

Ah, thanks for digging into that! That's good to know.

As for closing multiple times, I'm not so sure about that. In this branch we don't return the response, we return an error and a nil response.

return nil, fmt.Errorf("bad response status code from http loader url: %v", resp.StatusCode)
}
return resp, nil
Expand Down

0 comments on commit 817470d

Please sign in to comment.