Assure we exhaust bodies of all HTTP responses.

Signed-off-by: bwplotka <bwplotka@gmail.com>

api/client.go

@@ -24,6 +24,8 @@ import (
 	"path"
 	"strings"
 	"time"
+
+	"github.com/prometheus/client_golang/internal/errcapture"
 )
 
 // DefaultRoundTripper is used if no RoundTripper is set in Config.
@@ -118,39 +120,17 @@ func (c *httpClient) URL(ep string, args map[string]string) *url.URL {
 	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
+	var buf bytes.Buffer
-	done := make(chan struct{})
+	_, err = buf.ReadFrom(resp.Body)
-	go func() {
+	return resp, buf.Bytes(), err
-		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
 }

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
+}

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
+		}
+	}
+}

prometheus/push/push.go

@@ -48,6 +48,7 @@ import (
 	"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 @@ func (p *Pusher) Format(format expfmt.Format) *Pusher {
 //
 // 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 @@ func (p *Pusher) Delete() error {
 	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 @@ func (p *Pusher) push(ctx context.Context, method string) error {
 	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.

prometheus/testutil/testutil.go

@@ -48,6 +48,7 @@ import (
 	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 @@ func GatherAndCount(g prometheus.Gatherer, metricNames ...string) (int, error) {
 // 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",