Assure we exhaust bodies of all HTTP responses. Signed-off-by: bwplotka <bwplotka@gmail.com>
diff --git a/api/client.go b/api/client.go index 72a0130..d3386c9 100644 --- a/api/client.go +++ b/api/client.go
@@ -24,6 +24,8 @@ "path" "strings" "time" + + "github.com/prometheus/client_golang/internal/errcapture" ) // DefaultRoundTripper is used if no RoundTripper is set in Config. @@ -118,39 +120,17 @@ return &u } -func (c *httpClient) Do(ctx context.Context, req *http.Request) (*http.Response, []byte, error) { +func (c *httpClient) Do(ctx context.Context, req *http.Request) (_ *http.Response, _ []byte, err error) { if ctx != nil { req = req.WithContext(ctx) } resp, err := c.client.Do(req) - defer func() { - if resp != nil { - resp.Body.Close() - } - }() - if err != nil { return nil, nil, err } + defer errcapture.ExhaustClose(&err, resp.Body, "close response body") - var body []byte - done := make(chan struct{}) - go func() { - var buf bytes.Buffer - _, err = buf.ReadFrom(resp.Body) - body = buf.Bytes() - close(done) - }() - - select { - case <-ctx.Done(): - <-done - err = resp.Body.Close() - if err == nil { - err = ctx.Err() - } - case <-done: - } - - return resp, body, err + var buf bytes.Buffer + _, err = buf.ReadFrom(resp.Body) + return resp, buf.Bytes(), err }
diff --git a/internal/errcapture/errcapture.go b/internal/errcapture/errcapture.go new file mode 100644 index 0000000..d098cf3 --- /dev/null +++ b/internal/errcapture/errcapture.go
@@ -0,0 +1,61 @@ +// Copyright 2014 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package errcapture + +import ( + "errors" + "fmt" + "io" + "io/ioutil" + "os" + + "github.com/prometheus/client_golang/prometheus" +) + +type doFunc func() error + +// Do runs function and on error return error by argument including the given error (usually +// from caller function). +func Do(err *error, doer doFunc, format string, a ...interface{}) { + derr := doer() + if err == nil || derr == nil { + return + } + + // For os closers, it's a common case to double close. + // From reliability purpose this is not a problem it may only indicate surprising execution path. + if errors.Is(derr, os.ErrClosed) { + return + } + + errs := prometheus.MultiError{} + errs.Append(*err) + errs.Append(fmt.Errorf(format+": %w", append(a, derr)...)) + *err = errs +} + +// ExhaustClose closes the io.ReadCloser with error capture but exhausts the reader before. +func ExhaustClose(err *error, r io.ReadCloser, format string, a ...interface{}) { + _, copyErr := io.Copy(ioutil.Discard, r) + + Do(err, r.Close, format, a...) + if copyErr == nil { + return + } + + errs := prometheus.MultiError{} + errs.Append(copyErr) + errs.Append(*err) + *err = errs +}
diff --git a/internal/errcapture/errcapture_test.go b/internal/errcapture/errcapture_test.go new file mode 100644 index 0000000..d7d93a8 --- /dev/null +++ b/internal/errcapture/errcapture_test.go
@@ -0,0 +1,82 @@ +// Copyright 2014 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package errcapture + +import ( + "errors" + "io" + "testing" +) + +type testCloser struct { + err error +} + +func (c testCloser) Close() error { + return c.err +} + +func TestDo(t *testing.T) { + for _, tcase := range []struct { + err error + closer io.Closer + + expectedErrStr string + }{ + { + err: nil, + closer: testCloser{err: nil}, + expectedErrStr: "", + }, + { + err: errors.New("test"), + closer: testCloser{err: nil}, + expectedErrStr: "test", + }, + { + err: nil, + closer: testCloser{err: errors.New("test")}, + expectedErrStr: "1 error(s) occurred:\n* close: test", + }, + { + err: errors.New("test"), + closer: testCloser{err: errors.New("test")}, + expectedErrStr: "2 error(s) occurred:\n* test\n* close: test", + }, + } { + if ok := t.Run("", func(t *testing.T) { + ret := tcase.err + Do(&ret, tcase.closer.Close, "close") + + if tcase.expectedErrStr == "" { + if ret != nil { + t.Error("Expected error to be nil") + t.Fail() + } + } else { + if ret == nil { + t.Error("Expected error to be not nil") + t.Fail() + } + + if tcase.expectedErrStr != ret.Error() { + t.Errorf("%s != %s", tcase.expectedErrStr, ret.Error()) + t.Fail() + } + } + }); !ok { + return + } + } +}
diff --git a/prometheus/push/push.go b/prometheus/push/push.go index 06dee37..08059e7 100644 --- a/prometheus/push/push.go +++ b/prometheus/push/push.go
@@ -48,6 +48,7 @@ "github.com/prometheus/common/expfmt" "github.com/prometheus/common/model" + "github.com/prometheus/client_golang/internal/errcapture" "github.com/prometheus/client_golang/prometheus" ) @@ -228,7 +229,7 @@ // // Delete returns the first error encountered by any method call (including this // one) in the lifetime of the Pusher. -func (p *Pusher) Delete() error { +func (p *Pusher) Delete() (err error) { if p.error != nil { return p.error } @@ -243,7 +244,8 @@ if err != nil { return err } - defer resp.Body.Close() + defer errcapture.ExhaustClose(&err, resp.Body, "close response body") + if resp.StatusCode != http.StatusAccepted { body, _ := io.ReadAll(resp.Body) // Ignore any further error as this is for an error message only. return fmt.Errorf("unexpected status code %d while deleting %s: %s", resp.StatusCode, p.fullURL(), body) @@ -294,7 +296,8 @@ if err != nil { return err } - defer resp.Body.Close() + defer errcapture.ExhaustClose(&err, resp.Body, "close response body") + // Depending on version and configuration of the PGW, StatusOK or StatusAccepted may be returned. if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusAccepted { body, _ := io.ReadAll(resp.Body) // Ignore any further error as this is for an error message only.
diff --git a/prometheus/testutil/testutil.go b/prometheus/testutil/testutil.go index 91b83b5..7c1aa48 100644 --- a/prometheus/testutil/testutil.go +++ b/prometheus/testutil/testutil.go
@@ -48,6 +48,7 @@ dto "github.com/prometheus/client_model/go" "github.com/prometheus/common/expfmt" + "github.com/prometheus/client_golang/internal/errcapture" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/internal" ) @@ -158,12 +159,12 @@ // ScrapeAndCompare calls a remote exporter's endpoint which is expected to return some metrics in // plain text format. Then it compares it with the results that the `expected` would return. // If the `metricNames` is not empty it would filter the comparison only to the given metric names. -func ScrapeAndCompare(url string, expected io.Reader, metricNames ...string) error { +func ScrapeAndCompare(url string, expected io.Reader, metricNames ...string) (err error) { resp, err := http.Get(url) if err != nil { return fmt.Errorf("scraping metrics failed: %w", err) } - defer resp.Body.Close() + defer errcapture.ExhaustClose(&err, resp.Body, "close response body") if resp.StatusCode != http.StatusOK { return fmt.Errorf("the scraping target returned a status code other than 200: %d",