Make sure we close HTTP response body (#815)

> If the returned error is nil, the [Response](https://pkg.go.dev/net/http#Response) will contain a non-nil Body which the user is expected to close. If the Body is not both read to EOF and closed, the [Client](https://pkg.go.dev/net/http#Client)'s underlying [RoundTripper](https://pkg.go.dev/net/http#RoundTripper) (typically [Transport](https://pkg.go.dev/net/http#Transport)) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.
>
> *-- https://pkg.go.dev/net/http#Client.Do*

But it also may leak resources 🤷 

See also:

 - https://blevesearch.com/news/Deferred-Cleanup,-Checking-Errors,-and-Potential-Problems/
 - https://stackoverflow.com/questions/47293975/should-i-error-check-close-on-a-response-body
 - https://manishrjain.com/must-close-golang-http-response

Author: MadLittleMods

client/client.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/matrix-org/complement/b"
 	"github.com/matrix-org/complement/ct"
+	"github.com/matrix-org/complement/internal"
 )
 
 type ctxKey string
@@ -668,6 +669,9 @@ func (c *CSAPI) MustDo(t ct.TestLike, method string, paths []string, opts ...Req
 //			match.JSONKeyEqual("errcode", "M_INVALID_USERNAME"),
 //		},
 //	})
+//
+// The caller does not need to worry about closing the returned `http.Response.Body` as
+// this is handled automatically.
 func (c *CSAPI) Do(t ct.TestLike, method string, paths []string, opts ...RequestOpt) *http.Response {
 	t.Helper()
 	escapedPaths := make([]string, len(paths))
@@ -716,6 +720,30 @@ func (c *CSAPI) Do(t ct.TestLike, method string, paths []string, opts ...Request
 		if err != nil {
 			ct.Fatalf(t, "CSAPI.Do response returned error: %s", err)
 		}
+		// `defer` is function scoped but it's okay that we only clean up all requests at
+		// the end. To also be clear, `defer` arguments are evaluated at the time of the
+		// `defer` statement so we are only closing the original response body here. Our new
+		// response body will be untouched.
+		defer internal.CloseIO(
+			res.Body,
+			fmt.Sprintf(
+				"CSAPI.Do: response body from %s %s",
+				res.Request.Method,
+				res.Request.URL.String(),
+			),
+		)
+
+		// Make a copy of the response body so that downstream callers can read it multiple
+		// times if needed and don't need to worry about closing it.
+		var resBody []byte
+		if res.Body != nil {
+			resBody, err = io.ReadAll(res.Body)
+			if err != nil {
+				ct.Fatalf(t, "CSAPI.Do failed to read response body for RetryUntil check: %s", err)
+			}
+			res.Body = io.NopCloser(bytes.NewBuffer(resBody))
+		}
+
 		// debug log the response
 		if c.Debug && res != nil {
 			var dump []byte
@@ -725,19 +753,12 @@ func (c *CSAPI) Do(t ct.TestLike, method string, paths []string, opts ...Request
 			}
 			t.Logf("%s", string(dump))
 		}
+
 		if retryUntil == nil || retryUntil.timeout == 0 {
 			return res // don't retry
 		}
 
-		// check the condition, make a copy of the response body first in case the check consumes it
-		var resBody []byte
-		if res.Body != nil {
-			resBody, err = io.ReadAll(res.Body)
-			if err != nil {
-				ct.Fatalf(t, "CSAPI.Do failed to read response body for RetryUntil check: %s", err)
-			}
-			res.Body = io.NopCloser(bytes.NewBuffer(resBody))
-		}
+		// check the condition
 		if retryUntil.untilFn(res) {
 			// remake the response and return
 			res.Body = io.NopCloser(bytes.NewBuffer(resBody))

cmd/account-snapshot/internal/sync.go

@@ -12,6 +12,8 @@ import (
 	"os"
 	"strconv"
 	"strings"
+
+	"github.com/matrix-org/complement/internal"
 )
 
 // LoadSyncData loads sync data from disk or by doing a /sync request
@@ -75,7 +77,14 @@ func doRequest(httpCli *http.Client, req *http.Request, token string) ([]byte, e
 	if err != nil {
 		return nil, fmt.Errorf("failed to perform request: %w", err)
 	}
-	defer res.Body.Close()
+	defer internal.CloseIO(
+		res.Body,
+		fmt.Sprintf(
+			"doRequest: response body from %s %s",
+			res.Request.Method,
+			res.Request.URL.String(),
+		),
+	)
 	if res.StatusCode != 200 {
 		return nil, fmt.Errorf("response returned %s", res.Status)
 	}

federation/server.go

@@ -3,6 +3,7 @@
 package federation
 
 import (
+	"bytes"
 	"context"
 	"crypto/ed25519"
 	"crypto/rand"
@@ -12,6 +13,7 @@ import (
 	"encoding/json"
 	"encoding/pem"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"math/big"
 	"net"
@@ -32,6 +34,7 @@ import (
 
 	"github.com/matrix-org/complement/config"
 	"github.com/matrix-org/complement/ct"
+	"github.com/matrix-org/complement/internal"
 )
 
 // Subset of Deployment used in federation
@@ -278,6 +281,9 @@ func (s *Server) SendFederationRequest(
 // DoFederationRequest signs and sends an arbitrary federation request from this server, and returns the response.
 //
 // The requests will be routed according to the deployment map in `deployment`.
+//
+// The caller does not need to worry about closing the returned `http.Response.Body` as
+// this is handled automatically.
 func (s *Server) DoFederationRequest(
 	ctx context.Context,
 	t ct.TestLike,
@@ -297,12 +303,25 @@ func (s *Server) DoFederationRequest(
 
 	var resp *http.Response
 	resp, err = httpClient.DoHTTPRequest(ctx, httpReq)
+	defer internal.CloseIO(resp.Body, "DoFederationRequest: federation response body")
 
 	if httpError, ok := err.(gomatrix.HTTPError); ok {
 		t.Logf("[SSAPI] %s %s%s => error(%d): %s (%s)", req.Method(), req.Destination(), req.RequestURI(), httpError.Code, err, time.Since(start))
 	} else if err == nil {
 		t.Logf("[SSAPI] %s %s%s => %d (%s)", req.Method(), req.Destination(), req.RequestURI(), resp.StatusCode, time.Since(start))
 	}
+
+	// Make a copy of the response body so that downstream callers can read it multiple
+	// times if needed and don't need to worry about closing it.
+	var respBody []byte
+	if resp.Body != nil {
+		respBody, err = io.ReadAll(resp.Body)
+		if err != nil {
+			ct.Fatalf(t, "CSAPI.Do failed to read response body for RetryUntil check: %s", err)
+		}
+		resp.Body = io.NopCloser(bytes.NewBuffer(respBody))
+	}
+
 	return resp, err
 }
 

federation/server_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/matrix-org/complement/config"
+	"github.com/matrix-org/complement/internal"
 )
 
 type fedDeploy struct {
@@ -63,10 +64,10 @@ func TestComplementServerIsSigned(t *testing.T) {
 				return // wanted failure, got failure
 			}
 		}
+		defer internal.CloseIO(resp.Body, "server response body")
 		if !tc.wantSuccess {
 			t.Fatalf("request succeeded when we expected it to fail")
 		}
-		defer resp.Body.Close()
 
 		if resp.StatusCode != 404 {
 			t.Errorf("expected 404, got %d", resp.StatusCode)

internal/docker/deployer.go

@@ -31,6 +31,7 @@ import (
 	"time"
 
 	"github.com/docker/docker/client"
+	"github.com/matrix-org/complement/internal"
 	complementRuntime "github.com/matrix-org/complement/runtime"
 
 	"github.com/docker/docker/api/types/container"
@@ -668,6 +669,7 @@ func waitForContainer(ctx context.Context, docker *client.Client, hsDep *Homeser
 			time.Sleep(50 * time.Millisecond)
 			continue
 		}
+		defer internal.CloseIO(res.Body, "waitForContainer: version response body")
 		if res.StatusCode != 200 {
 			lastErr = fmt.Errorf("GET %s => HTTP %s", versionsURL, res.Status)
 			time.Sleep(50 * time.Millisecond)

internal/instruction/runner.go

@@ -18,6 +18,7 @@ import (
 	"github.com/tidwall/gjson"
 
 	"github.com/matrix-org/complement/b"
+	"github.com/matrix-org/complement/internal"
 )
 
 // An instruction for the runner to run.
@@ -212,6 +213,15 @@ func (r *Runner) runInstructionSet(contextStr string, hsURL string, instrs []ins
 				return err
 			}
 		}
+		defer internal.CloseIO(
+			res.Body,
+			fmt.Sprintf(
+				"runInstructionSet: response body from %s %s",
+				res.Request.Method,
+				res.Request.URL.String(),
+			),
+		)
+
 		// parse the response if we have one (if bestEffort=true then we don't return an error above)
 		if res != nil && res.Body != nil {
 			if i < 100 || i%200 == 0 {

internal/io.go

@@ -0,0 +1,43 @@
+package internal
+
+import (
+	"io"
+	"log"
+)
+
+// CloseIO is a little helper to close an io.Closer and log any error encountered.
+//
+// Based off of https://blevesearch.com/news/Deferred-Cleanup,-Checking-Errors,-and-Potential-Problems/
+//
+// Probably, most relevant for closing HTTP response bodies as they MUST be closed, even
+// if you don’t read it. https://manishrjain.com/must-close-golang-http-response
+//
+// Usage:
+// ```go
+// res, err := client.Do(req)
+// defer internal.CloseIO(res.Body, "request body")
+// ```
+//
+// Alternative to this bulky pattern:
+//
+// ```go
+// res, err := client.Do(req)
+// defer func(c io.Closer) {
+// 	if c != nil {
+// 		err := c.Close()
+// 		if err != nil {
+// 			log.Fatalf("error closing request body stream %v", err)
+// 		}
+// 	}
+// }(res.Body)
+// ```
+func CloseIO(c io.Closer, contextString string) {
+	if c != nil {
+		err := c.Close()
+		if err != nil {
+			// In most cases, not much we can do besides logging as we already received and
+			// handled whatever resource this io.Closer was wrapping.
+			log.Fatalf("error closing io.Closer (%s): %v", contextString, err)
+		}
+	}
+}

tests/federation_keys_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/tidwall/sjson"
 
 	"github.com/matrix-org/complement"
+	"github.com/matrix-org/complement/internal"
 	"github.com/matrix-org/complement/match"
 	"github.com/matrix-org/complement/must"
 )
@@ -36,6 +37,7 @@ func TestInboundFederationKeys(t *testing.T) {
 
 	res, err := fedClient.Get("https://hs1/_matrix/key/v2/server")
 	must.NotError(t, "failed to GET /keys", err)
+	defer internal.CloseIO(res.Body, "server key response body")
 
 	var keys = map[string]ed25519.PublicKey{}
 	var oldKeys = map[string]ed25519.PublicKey{}