From bb1cbcbd060dd8c6eefe77bf1b0281f08f0d33a6 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 5 Nov 2025 17:10:30 -0600 Subject: [PATCH 1/4] Make sure we close HTTP response body --- client/client.go | 17 +++++++++++ cmd/account-snapshot/internal/sync.go | 11 ++++++- federation/server.go | 19 ++++++++++++ federation/server_test.go | 3 +- internal/docker/deployer.go | 2 ++ internal/instruction/runner.go | 10 +++++++ internal/io.go | 43 +++++++++++++++++++++++++++ tests/federation_keys_test.go | 2 ++ 8 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 internal/io.go diff --git a/client/client.go b/client/client.go index b4d3e14e..56de7919 100644 --- a/client/client.go +++ b/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)) @@ -713,9 +717,22 @@ func (c *CSAPI) Do(t ct.TestLike, method string, paths []string, opts ...Request for { // Perform the HTTP request res, err := c.Client.Do(req) + // `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(), + ), + ) if err != nil { ct.Fatalf(t, "CSAPI.Do response returned error: %s", err) } + // debug log the response if c.Debug && res != nil { var dump []byte diff --git a/cmd/account-snapshot/internal/sync.go b/cmd/account-snapshot/internal/sync.go index 616f5f07..2f50b001 100644 --- a/cmd/account-snapshot/internal/sync.go +++ b/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 @@ -72,10 +74,17 @@ func loadDataFromDisk(tempFile string) json.RawMessage { func doRequest(httpCli *http.Client, req *http.Request, token string) ([]byte, error) { req.Header.Set("Authorization", "Bearer "+token) res, err := httpCli.Do(req) + defer internal.CloseIO( + res.Body, + fmt.Sprintf( + "doRequest: response body from %s %s", + res.Request.Method, + res.Request.URL.String(), + ), + ) if err != nil { return nil, fmt.Errorf("failed to perform request: %w", err) } - defer res.Body.Close() if res.StatusCode != 200 { return nil, fmt.Errorf("response returned %s", res.Status) } diff --git a/federation/server.go b/federation/server.go index d8825cc1..ba7478de 100644 --- a/federation/server.go +++ b/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 } diff --git a/federation/server_test.go b/federation/server_test.go index 3db645fb..657523ca 100644 --- a/federation/server_test.go +++ b/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 { @@ -56,6 +57,7 @@ func TestComplementServerIsSigned(t *testing.T) { client := &http.Client{Transport: transport} resp, err := client.Get("https://" + string(srv.ServerName())) + defer internal.CloseIO(resp.Body, "server response body") if err != nil { if tc.wantSuccess { t.Fatalf("Failed to GET: %s", err) @@ -66,7 +68,6 @@ func TestComplementServerIsSigned(t *testing.T) { 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) diff --git a/internal/docker/deployer.go b/internal/docker/deployer.go index 6ce9dc33..b6922920 100644 --- a/internal/docker/deployer.go +++ b/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" @@ -663,6 +664,7 @@ func waitForContainer(ctx context.Context, docker *client.Client, hsDep *Homeser break } res, err := http.Get(versionsURL) + defer internal.CloseIO(res.Body, "waitForContainer: version response body") if err != nil { lastErr = fmt.Errorf("GET %s => error: %s", versionsURL, err) time.Sleep(50 * time.Millisecond) diff --git a/internal/instruction/runner.go b/internal/instruction/runner.go index 16db45f5..912f81a2 100644 --- a/internal/instruction/runner.go +++ b/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. @@ -206,12 +207,21 @@ func (r *Runner) runInstructionSet(contextStr string, hsURL string, instrs []ins return fmt.Errorf("terminated") } res, err := cli.Do(req) + defer internal.CloseIO( + res.Body, + fmt.Sprintf( + "runInstructionSet: response body from %s %s", + res.Request.Method, + res.Request.URL.String(), + ), + ) if err != nil { err = isFatalErr(fmt.Errorf("%s : failed to perform HTTP request to %s with body %+v: %w", contextStr, req.URL.String(), instr.body, err)) if err != nil { return err } } + // 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 { diff --git a/internal/io.go b/internal/io.go new file mode 100644 index 00000000..77d46f41 --- /dev/null +++ b/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) + } + } +} diff --git a/tests/federation_keys_test.go b/tests/federation_keys_test.go index 7907b191..1be7d806 100644 --- a/tests/federation_keys_test.go +++ b/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" ) @@ -35,6 +36,7 @@ func TestInboundFederationKeys(t *testing.T) { } res, err := fedClient.Get("https://hs1/_matrix/key/v2/server") + defer internal.CloseIO(res.Body, "server key response body") must.NotError(t, "failed to GET /keys", err) var keys = map[string]ed25519.PublicKey{} From 3539f1b95cf8a2149c9ee44e3f7a76573d554025 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 5 Nov 2025 17:41:37 -0600 Subject: [PATCH 2/4] Fix client closing body for non-retried requests --- client/client.go | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/client/client.go b/client/client.go index 56de7919..0d9e8ef4 100644 --- a/client/client.go +++ b/client/client.go @@ -733,6 +733,17 @@ func (c *CSAPI) Do(t ct.TestLike, method string, paths []string, opts ...Request ct.Fatalf(t, "CSAPI.Do response returned error: %s", err) } + // 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 @@ -742,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)) From 92f3cf20c42aff6f22cb9498db6b27605dca946c Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 5 Nov 2025 17:50:09 -0600 Subject: [PATCH 3/4] `defer` after error check as `res` can be `nil` --- client/client.go | 6 +++--- cmd/account-snapshot/internal/sync.go | 6 +++--- federation/server_test.go | 2 +- internal/docker/deployer.go | 2 +- internal/instruction/runner.go | 12 ++++++------ tests/federation_keys_test.go | 2 +- 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/client/client.go b/client/client.go index 0d9e8ef4..e43ebce4 100644 --- a/client/client.go +++ b/client/client.go @@ -717,6 +717,9 @@ func (c *CSAPI) Do(t ct.TestLike, method string, paths []string, opts ...Request for { // Perform the HTTP request res, err := c.Client.Do(req) + 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 @@ -729,9 +732,6 @@ func (c *CSAPI) Do(t ct.TestLike, method string, paths []string, opts ...Request res.Request.URL.String(), ), ) - if err != nil { - ct.Fatalf(t, "CSAPI.Do response returned error: %s", err) - } // 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. diff --git a/cmd/account-snapshot/internal/sync.go b/cmd/account-snapshot/internal/sync.go index 2f50b001..278ceb7e 100644 --- a/cmd/account-snapshot/internal/sync.go +++ b/cmd/account-snapshot/internal/sync.go @@ -74,6 +74,9 @@ func loadDataFromDisk(tempFile string) json.RawMessage { func doRequest(httpCli *http.Client, req *http.Request, token string) ([]byte, error) { req.Header.Set("Authorization", "Bearer "+token) res, err := httpCli.Do(req) + if err != nil { + return nil, fmt.Errorf("failed to perform request: %w", err) + } defer internal.CloseIO( res.Body, fmt.Sprintf( @@ -82,9 +85,6 @@ func doRequest(httpCli *http.Client, req *http.Request, token string) ([]byte, e res.Request.URL.String(), ), ) - if err != nil { - return nil, fmt.Errorf("failed to perform request: %w", err) - } if res.StatusCode != 200 { return nil, fmt.Errorf("response returned %s", res.Status) } diff --git a/federation/server_test.go b/federation/server_test.go index 657523ca..33561937 100644 --- a/federation/server_test.go +++ b/federation/server_test.go @@ -57,7 +57,6 @@ func TestComplementServerIsSigned(t *testing.T) { client := &http.Client{Transport: transport} resp, err := client.Get("https://" + string(srv.ServerName())) - defer internal.CloseIO(resp.Body, "server response body") if err != nil { if tc.wantSuccess { t.Fatalf("Failed to GET: %s", err) @@ -65,6 +64,7 @@ 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") } diff --git a/internal/docker/deployer.go b/internal/docker/deployer.go index b6922920..0a8a511a 100644 --- a/internal/docker/deployer.go +++ b/internal/docker/deployer.go @@ -664,12 +664,12 @@ func waitForContainer(ctx context.Context, docker *client.Client, hsDep *Homeser break } res, err := http.Get(versionsURL) - defer internal.CloseIO(res.Body, "waitForContainer: version response body") if err != nil { lastErr = fmt.Errorf("GET %s => error: %s", versionsURL, err) 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) diff --git a/internal/instruction/runner.go b/internal/instruction/runner.go index 912f81a2..d2f14214 100644 --- a/internal/instruction/runner.go +++ b/internal/instruction/runner.go @@ -207,6 +207,12 @@ func (r *Runner) runInstructionSet(contextStr string, hsURL string, instrs []ins return fmt.Errorf("terminated") } res, err := cli.Do(req) + if err != nil { + err = isFatalErr(fmt.Errorf("%s : failed to perform HTTP request to %s with body %+v: %w", contextStr, req.URL.String(), instr.body, err)) + if err != nil { + return err + } + } defer internal.CloseIO( res.Body, fmt.Sprintf( @@ -215,12 +221,6 @@ func (r *Runner) runInstructionSet(contextStr string, hsURL string, instrs []ins res.Request.URL.String(), ), ) - if err != nil { - err = isFatalErr(fmt.Errorf("%s : failed to perform HTTP request to %s with body %+v: %w", contextStr, req.URL.String(), instr.body, err)) - if err != nil { - return err - } - } // parse the response if we have one (if bestEffort=true then we don't return an error above) if res != nil && res.Body != nil { diff --git a/tests/federation_keys_test.go b/tests/federation_keys_test.go index 1be7d806..316927a5 100644 --- a/tests/federation_keys_test.go +++ b/tests/federation_keys_test.go @@ -36,8 +36,8 @@ func TestInboundFederationKeys(t *testing.T) { } res, err := fedClient.Get("https://hs1/_matrix/key/v2/server") - defer internal.CloseIO(res.Body, "server key response body") 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{} From 61ff0b18947f92dbf81ff12752e7a31c0617e1f4 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 10 Nov 2025 09:54:07 -0600 Subject: [PATCH 4/4] Fix indentation in example See https://github.com/matrix-org/complement/pull/815#discussion_r2509569693 --- internal/io.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/io.go b/internal/io.go index 77d46f41..fac5f02d 100644 --- a/internal/io.go +++ b/internal/io.go @@ -22,14 +22,14 @@ import ( // // ```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) +// 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 {