Repository: bazelbuild/bazelisk
Pull request #624 (2024-10-23 14:42:29)

feat: improve httputil error messages
* add attempt count to the timeout error message so it's clear the timeout is due to the total time accumulated during retries.

* add lastFailure to the timeout error message so we can get a clear reason for the retries (or, at the very least, the last retry). Otherwise, the error message obscures the real reason(s) for the retries and potentially misleads users into thinking that the errors could be e.g. network related (more typical of a timeout) rather than other types of errors (e.g. "tls: failed to verify certificate", etc.)

jjmaestro: The motivation behind this PR was my own debugging creating a Docker image where I was getting constant timeouts and I eventually figured it was due to not having `ca-certificates` installed in my Debian-based image. I debugged it by patching the error messages with this PR. 

I hope it's also useful for others!

Patch:
diff --git a/deb/BUILD b/deb/BUILD
index a74f52b..6a32ce8 100644
--- a/deb/BUILD
+++ b/deb/BUILD
@@ -35,14 +35,18 @@ ARCHES = ["amd64", "arm64"]
 [
     pkg_deb(
         name = "bazelisk-{}_deb".format(arch),
+        out = "bazelisk-{}.deb".format(arch),
         architecture = arch,
-        conflicts = ["bazel"],
+        conflicts = [
+            "bazel",
+            "bazel-bootstrap",
+        ],
         data = ":bazelisk-{}_tar".format(arch),
+        depends = ["ca-certificates"],
         description_file = ":description.txt",
         homepage = "https://github.com/bazelbuild/bazelisk",
         license = "Apache-2.0",
         maintainer = "The Bazel Authors <bazel-dev@googlegroups.com>",
-        out = "bazelisk-{}.deb".format(arch),
         package = "bazelisk",
         section = "contrib/devel",
         version_file = ":version.txt",
diff --git a/httputil/httputil.go b/httputil/httputil.go
index 2180ce9..c020e06 100644
--- a/httputil/httputil.go
+++ b/httputil/httputil.go
@@ -95,6 +95,12 @@ func get(url, auth string) (*http.Response, error) {
 			return res, err
 		}
 
+		if (res != nil) {
+			// Need to retry, close the response body immediately to release resources.
+			// See https://github.com/googleapis/google-cloud-go/issues/7440#issuecomment-1491008639
+			res.Body.Close()
+		}
+
 		if err == nil {
 			lastFailure = fmt.Sprintf("HTTP %d", res.StatusCode)
 		} else {
@@ -107,7 +113,7 @@ func get(url, auth string) (*http.Response, error) {
 
 		nextTryAt := RetryClock.Now().Add(waitFor)
 		if nextTryAt.After(deadline) {
-			return nil, fmt.Errorf("unable to complete request to %s within %v", url, MaxRequestDuration)
+			return nil, fmt.Errorf("unable to complete %d requests to %s within %v. Most recent failure: %s", attempt+1, url, MaxRequestDuration, lastFailure)
 		}
 		if attempt < MaxRetries {
 			RetryClock.Sleep(waitFor)
@@ -121,7 +127,7 @@ func shouldRetry(res *http.Response, err error) bool {
 	if err != nil {
 		return true
 	}
-	// For HTTP: only retry on permanent/fatal errors.
+	// For HTTP: only retry on non-permanent/fatal errors.
 	return res.StatusCode == 429 || (500 <= res.StatusCode && res.StatusCode <= 504)
 }
 


Test patch:
diff --git a/httputil/httputil_test.go b/httputil/httputil_test.go
index 254e429..1a3f228 100644
--- a/httputil/httputil_test.go
+++ b/httputil/httputil_test.go
@@ -4,6 +4,7 @@ import (
 	"errors"
 	"net/http"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
 )
@@ -219,10 +220,13 @@ func TestDeadlineExceeded(t *testing.T) {
 		t.Fatal("Expected request to fail with code 500")
 	}
 
-	wanted := "could not fetch http://bar: unable to complete request to http://bar within 8s"
+	wantedPrefix := "could not fetch http://bar: unable to complete "
+	wantedSuffix := " requests to http://bar within 8s. Most recent failure: HTTP 500"
+
 	got := err.Error()
-	if wanted != got {
-		t.Fatalf("Expected error %q, but got %q", wanted, got)
+	sameError := strings.HasPrefix(got, wantedPrefix) && strings.HasSuffix(got, wantedSuffix)
+	if !sameError {
+		t.Fatalf("Expected error %q, but got %q", wantedPrefix+"???"+wantedSuffix, got)
 	}
 }