chore: close http responses in a way to allow the Transport to re-use the TCP connection

[atzoum]

Description

If the http response body is not read, the underlying TCP connection cannot be reused (ref).

For that purpose, introducing httputil.CloseResponse

// CloseResponse closes the response's body. But reads at least some of the body so if it's
// small the underlying TCP connection will be re-used. No need to check for errors: if it
// fails, the Transport won't reuse it anyway.
func CloseResponse(resp *http.Response) {
	if resp != nil && resp.Body != nil {
		const maxBodySlurpSize = 2 << 10 // 2KB
		_, _ = io.CopyN(io.Discard, resp.Body, maxBodySlurpSize)
		resp.Body.Close()
	}
}

Note
The usage patterns of this new function might seem strange to you at first, since instead of

defer httputil.CloseResponse(resp)
// or
httputil.CloseResponse(resp)

we are doing

defer func() { httputil.CloseResponse(resp) }()
// or 
func() { httputil.CloseResponse(resp) }()

The justification for using the above patterns is that golangci-lint bodyclose linter, doesn't seem to recognise an indirect body close operation unless it is performed within a closure!!

Notion Ticket

Link

Security

• The code changed/added as part of this pull request won't create any security issues with how the software is being used.

[lvrach]

Two concerns:

1. I don't find the usage of httputil.CloseResponse everywhere very useful; if you plan to read the body anyway, what it the point of using this function? I believe it will unnecessarily complicate things. I prefer to use it only if we plan to discard the body.
2. The usage of const maxBodySlurpSize = 2 << 10 // 2KB is an implementation detail which makes sense to be part of HTTP package, but I would refrain from hardcoding in our code.

My suggestion is the following:

Introducing this function:

func DiscardBody(resp *http.Response) {
	if resp != nil && resp.Body != nil {
		_, _ = io.Copy(io.Discard, resp.Body)
		resp.Body.Close()
	}
}

Use this function when we don't plan to read the body.

[lvrach]

Explaining myself a bit further:

• Adding a new function call in every HTTP request introduces overhead in all development cycles.
• The typical case should be to read the body of a request. Not reading the body is the exception.
• I don't want to enforce the usage of a call in every call to handle the exception, but I like the idea of making it easy and readable for people to discard it.

The biggest issue is that we can not enforce the usage of those functions. The ideal solution is to have a linter that requires you to read and close the body.

[atzoum]

I don't find the usage of httputil.CloseResponse everywhere very useful; if you plan to read the body anyway, what it the point of using this function? I believe it will unnecessarily complicate things. I prefer to use it only if we plan to discard the body.

I don't see any complication in always trying to read the request body just before closing it, even if it was already read before (resulting in a no-op). If this pattern is used across our codebase, it becomes easier for less senior members of the team to pick it up, copy-paste it and guard themselves from the inevitable complex branching logic which can result in not reading the body after all, even though this was the initial intention :)

Edit: It is not uncommon either for us to care about reading an HTTP response's body in case of a successful 2XX response. But this doesn't mean that unsuccessful responses don't contain a body. We need to consume bodies regardless if we care about them or not. Examples here & here

[atzoum]

2. The usage of const maxBodySlurpSize = 2 << 10 // 2KB is an implementation detail which makes sense to be part of HTTP package, but I would refrain from hardcoding in our code.

You caught me! I found it smart though, not attempting to read larger bodies which would result in more time and resources being spent compared to simply opening a new tcp connection ;)

[lvrach]

I don't find the usage of httputil.CloseResponse everywhere very useful; if you plan to read the body anyway, what it the point of using this function? I believe it will unnecessarily complicate things. I prefer to use it only if we plan to discard the body.

I don't see any complication in always trying to read the request body just before closing it, even if it was already read before (resulting in a no-op). If this pattern is used across our codebase, it becomes easier for less senior members of the team to pick it up, copy-paste it and guard themselves from the inevitable complex branching logic which can result in not reading the body after all, even though this was the initial intention :)

That is a good point. I have no strong argument; both approaches help. I will wait for @fracasula feedback.

[codecov]

Codecov Report

Base: 46.73% // Head: 46.87% // Increases project coverage by +0.13% 🎉

Coverage data is based on head (534ac62) compared to base (bed58c5).
Patch coverage: 65.11% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2718      +/-   ##
==========================================
+ Coverage   46.73%   46.87%   +0.13%     
==========================================
  Files         300      300              
  Lines       49123    49109      -14     
==========================================
+ Hits        22958    23018      +60     
+ Misses      24704    24624      -80     
- Partials     1461     1467       +6     

Impacted Files	Coverage Δ
enterprise/reporting/reporting.go	15.65% <0.00%> (ø)
event-schema/event_schema.go	40.88% <0.00%> (ø)
gateway/gateway.go	63.37% <0.00%> (+0.05%)	⬆️
services/alert/pagerduty.go	0.00% <0.00%> (ø)
services/alert/victorops.go	0.00% <0.00%> (ø)
utils/misc/misc.go	45.49% <0.00%> (+0.10%)	⬆️
warehouse/testhelper/events.go	0.00% <0.00%> (ø)
warehouse/testhelper/setup.go	0.00% <0.00%> (ø)
warehouse/utils/utils.go	71.10% <0.00%> (+0.26%)	⬆️
config/backend-config/namespace_config.go	67.00% <100.00%> (-3.00%)	⬇️
... and 33 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[fracasula]

I don't have a problem with always closing the body. Consistency can actually help in making sure we forget less. We're going to forget though. In this regard I think that some kind of linter as @atzoum proposed would be helpful in this case. Worst case we end up with what we have now (non-reusable connection), but writing a linter doesn't seem complex and it's definitely know-how that can turn out to be useful moving forward (and can be useful to others since it can be added to golangci-lint which would also give us visibility).

Another thing related to this PR, I'd report the bodyclose issue, it's silly that we have to wrap the close in an anonymous function.

Also another thing is that some defers are inside for loops and that is not a good practice (possible resource leak). Whenever possible please create a dedicated function or an anonymous one to execute the defer asap instead of having defer calls piling up for each element we iterate over.

for i := 0; i < c.Int("count"); i++ {
	/* ... */

	resp, err := client.Do(req)
	if err != nil {
		return err
	}
	defer func() { httputil.CloseResponse(resp) }()
	b, err := io.ReadAll(resp.Body)
	if err != nil {
		return err
	}

	if resp.StatusCode != http.StatusOK {
		fmt.Printf("%s\n%s\n", resp.Status, b)

		return fmt.Errorf("status code: %d", resp.StatusCode)
	}
}

Becomes:

for i := 0; i < c.Int("count"); i++ {
	/* ... */

	resp, b, err := doReq(client, req)
	if err != nil {
		return err
	}

	if resp.StatusCode != http.StatusOK {
		fmt.Printf("%s\n%s\n", resp.Status, b)

		return fmt.Errorf("status code: %d", resp.StatusCode)
	}
}

// ...

func doReq(client *http.Client, req *http.Request) (*http.Response, []byte, error) {
	resp, err := client.Do(req)
	if err != nil {
		return nil, nil, err
	}
	defer func() { httputil.CloseResponse(resp) }()
	b, err := io.ReadAll(resp.Body)
	if err != nil {
		return nil, nil, err
	}
	return resp, b, nil
}

[atzoum]

some defers are inside for loops

I could spot two occurrences of defers inside for loops. Please highlight any additional issues

[atzoum]

I'd report the bodyclose issue

I am afraid the linter's repo doesn't appear to be maintained anymore. It's automated build is failing and last commit is since 2021. There are also similar issues reported here and here without them receiving any response from the maintainer.

IMO, our best bet would be to move forward with our own linter, forking bodyclose, but this will take me some time

[fracasula]

Yeah I didn't realise it wasn't maintained anymore, that is unfortunate.