From b1a8fd1d03928673e024f96e7fc9cf62fcbd90b2 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 6 Jan 2024 15:24:33 +0100 Subject: [PATCH 1/2] rest: fix and cleanup closing of http response body If client.Do returns an error, then there's no body that has to be closed. For requests for which we are not interested in the response body, immediately drain and close the body to make sure it isn't forgotten later on. This change in particular adds the missing `Close()` call for the `List()` command. --- internal/backend/rest/rest.go | 79 +++++++++++++++++------------------ 1 file changed, 38 insertions(+), 41 deletions(-) diff --git a/internal/backend/rest/rest.go b/internal/backend/rest/rest.go index 5310eba7c..d8171d90e 100644 --- a/internal/backend/rest/rest.go +++ b/internal/backend/rest/rest.go @@ -58,6 +58,17 @@ func Open(_ context.Context, cfg Config, rt http.RoundTripper) (*Backend, error) return be, nil } +func drainAndClose(resp *http.Response) error { + _, err := io.Copy(io.Discard, resp.Body) + cerr := resp.Body.Close() + + // return first error + if err != nil { + return errors.Errorf("drain: %w", err) + } + return cerr +} + // Create creates a new REST on server configured in config. func Create(ctx context.Context, cfg Config, rt http.RoundTripper) (*Backend, error) { be, err := Open(ctx, cfg, rt) @@ -80,20 +91,14 @@ func Create(ctx context.Context, cfg Config, rt http.RoundTripper) (*Backend, er return nil, err } + if err := drainAndClose(resp); err != nil { + return nil, err + } + if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("server response unexpected: %v (%v)", resp.Status, resp.StatusCode) } - _, err = io.Copy(io.Discard, resp.Body) - if err != nil { - return nil, err - } - - err = resp.Body.Close() - if err != nil { - return nil, err - } - return be, nil } @@ -136,22 +141,19 @@ func (b *Backend) Save(ctx context.Context, h backend.Handle, rd backend.RewindR req.ContentLength = rd.Length() resp, err := b.client.Do(req) - - var cerr error - if resp != nil { - _, _ = io.Copy(io.Discard, resp.Body) - cerr = resp.Body.Close() - } - if err != nil { return errors.WithStack(err) } + if err := drainAndClose(resp); err != nil { + return err + } + if resp.StatusCode != http.StatusOK { return errors.Errorf("server response unexpected: %v (%v)", resp.Status, resp.StatusCode) } - return errors.Wrap(cerr, "Close") + return nil } // notExistError is returned whenever the requested file does not exist on the @@ -215,22 +217,17 @@ func (b *Backend) openReader(ctx context.Context, h backend.Handle, length int, req.Header.Set("Accept", ContentTypeV2) resp, err := b.client.Do(req) - if err != nil { - if resp != nil { - _, _ = io.Copy(io.Discard, resp.Body) - _ = resp.Body.Close() - } return nil, errors.Wrap(err, "client.Do") } if resp.StatusCode == http.StatusNotFound { - _ = resp.Body.Close() + _ = drainAndClose(resp) return nil, ¬ExistError{h} } if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusPartialContent { - _ = resp.Body.Close() + _ = drainAndClose(resp) return nil, errors.Errorf("unexpected HTTP response (%v): %v", resp.StatusCode, resp.Status) } @@ -250,13 +247,11 @@ func (b *Backend) Stat(ctx context.Context, h backend.Handle) (backend.FileInfo, return backend.FileInfo{}, errors.WithStack(err) } - _, _ = io.Copy(io.Discard, resp.Body) - if err = resp.Body.Close(); err != nil { - return backend.FileInfo{}, errors.Wrap(err, "Close") + if err = drainAndClose(resp); err != nil { + return backend.FileInfo{}, err } if resp.StatusCode == http.StatusNotFound { - _ = resp.Body.Close() return backend.FileInfo{}, ¬ExistError{h} } @@ -285,13 +280,15 @@ func (b *Backend) Remove(ctx context.Context, h backend.Handle) error { req.Header.Set("Accept", ContentTypeV2) resp, err := b.client.Do(req) - if err != nil { return errors.Wrap(err, "client.Do") } + if err = drainAndClose(resp); err != nil { + return err + } + if resp.StatusCode == http.StatusNotFound { - _ = resp.Body.Close() return ¬ExistError{h} } @@ -299,12 +296,7 @@ func (b *Backend) Remove(ctx context.Context, h backend.Handle) error { return errors.Errorf("blob not removed, server response: %v (%v)", resp.Status, resp.StatusCode) } - _, err = io.Copy(io.Discard, resp.Body) - if err != nil { - return errors.Wrap(err, "Copy") - } - - return errors.Wrap(resp.Body.Close(), "Close") + return nil } // List runs fn for each file in the backend which has the type t. When an @@ -322,7 +314,6 @@ func (b *Backend) List(ctx context.Context, t backend.FileType, fn func(backend. req.Header.Set("Accept", ContentTypeV2) resp, err := b.client.Do(req) - if err != nil { return errors.Wrap(err, "List") } @@ -333,19 +324,25 @@ func (b *Backend) List(ctx context.Context, t backend.FileType, fn func(backend. // already ignores missing directories, but misuses "not found" to // report certain internal errors, see // https://github.com/rclone/rclone/pull/7550 for details. - return nil + return drainAndClose(resp) } } if resp.StatusCode != http.StatusOK { + _ = drainAndClose(resp) return errors.Errorf("List failed, server response: %v (%v)", resp.Status, resp.StatusCode) } if resp.Header.Get("Content-Type") == ContentTypeV2 { - return b.listv2(ctx, resp, fn) + err = b.listv2(ctx, resp, fn) + } else { + err = b.listv1(ctx, t, resp, fn) } - return b.listv1(ctx, t, resp, fn) + if cerr := drainAndClose(resp); cerr != nil && err == nil { + err = cerr + } + return err } // listv1 uses the REST protocol v1, where a list HTTP request (e.g. `GET From bd883caae1333b99f7cec82b51c6945410cde83e Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 6 Jan 2024 15:38:57 +0100 Subject: [PATCH 2/2] CI: enable bodyclose linter --- .golangci.yml | 3 +++ internal/backend/limiter/static_limiter_test.go | 1 + internal/backend/rclone/backend.go | 1 + 3 files changed, 5 insertions(+) diff --git a/.golangci.yml b/.golangci.yml index 98b5f9e03..c08331401 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -35,6 +35,9 @@ linters: # parse and typecheck code - typecheck + # ensure that http response bodies are closed + - bodyclose + issues: # don't use the default exclude rules, this hides (among others) ignored # errors from Close() calls diff --git a/internal/backend/limiter/static_limiter_test.go b/internal/backend/limiter/static_limiter_test.go index 8a839518f..79a1d02f3 100644 --- a/internal/backend/limiter/static_limiter_test.go +++ b/internal/backend/limiter/static_limiter_test.go @@ -118,6 +118,7 @@ func TestRoundTripperReader(t *testing.T) { test.Assert(t, bytes.Equal(data, out.Bytes()), "data ping-pong failed") } +// nolint:bodyclose // the http response is just a mock func TestRoundTripperCornerCases(t *testing.T) { limiter := NewStaticLimiter(Limits{42 * 1024, 42 * 1024}) diff --git a/internal/backend/rclone/backend.go b/internal/backend/rclone/backend.go index a41a89898..416162364 100644 --- a/internal/backend/rclone/backend.go +++ b/internal/backend/rclone/backend.go @@ -252,6 +252,7 @@ func newBackend(ctx context.Context, cfg Config, lim limiter.Limiter) (*Backend, return nil, fmt.Errorf("error talking HTTP to rclone: %w", err) } + _ = res.Body.Close() debug.Log("HTTP status %q returned, moving instance to background", res.Status) err = bg() if err != nil {