Merge branch '21227-keep-web-panic'

[arvados.git] / sdk / go / arvados / client.go

diff --git a/sdk/go/arvados/client.go b/sdk/go/arvados/client.go
index 8a7c7fa60f82d056c1198380cc01d11776d095d7..e3c14326600189ed92f0b5af14db83f244d010f5 100644 (file)
--- a/sdk/go/arvados/client.go
+++ b/sdk/go/arvados/client.go
@@ -242,6 +242,8 @@ var reqIDGen = httpserver.IDGenerator{Prefix: "req-"}
 
 var nopCancelFunc context.CancelFunc = func() {}
 
+var reqErrorRe = regexp.MustCompile(`net/http: invalid header `)
+
 // Do augments (*http.Client)Do(): adds Authorization and X-Request-Id
 // headers, delays in order to comply with rate-limiting restrictions,
 // and retries failed requests when appropriate.
@@ -278,6 +280,7 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) {
        var lastResp *http.Response
        var lastRespBody io.ReadCloser
        var lastErr error
+       var checkRetryCalled int
 
        rclient := retryablehttp.NewClient()
        rclient.HTTPClient = c.httpClient()
@@ -291,11 +294,15 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) {
                rclient.RetryMax = 0
        }
        rclient.CheckRetry = func(ctx context.Context, resp *http.Response, respErr error) (bool, error) {
+               checkRetryCalled++
                if c.getRequestLimiter().Report(resp, respErr) {
                        c.last503.Store(time.Now())
                }
                if c.Timeout == 0 {
-                       return false, err
+                       return false, nil
+               }
+               if respErr != nil && reqErrorRe.MatchString(respErr.Error()) {
+                       return false, nil
                }
                retrying, err := retryablehttp.DefaultRetryPolicy(ctx, resp, respErr)
                if retrying {
@@ -327,7 +334,14 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) {
        }
        resp, err := rclient.Do(rreq)
        if (errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled)) && (lastResp != nil || lastErr != nil) {
-               resp, err = lastResp, lastErr
+               resp = lastResp
+               err = lastErr
+               if checkRetryCalled > 0 && err != nil {
+                       // Mimic retryablehttp's "giving up after X
+                       // attempts" message, even if we gave up
+                       // because of time rather than maxretries.
+                       err = fmt.Errorf("%s %s giving up after %d attempt(s): %w", req.Method, req.URL.String(), checkRetryCalled, err)
+               }
                if resp != nil {
                        resp.Body = lastRespBody
                }
@@ -648,7 +662,11 @@ func (c *Client) apiURL(path string) string {
        if scheme == "" {
                scheme = "https"
        }
-       return scheme + "://" + c.APIHost + "/" + path
+       // Double-slash in URLs tend to cause subtle hidden problems
+       // (e.g., they can behave differently when a load balancer is
+       // in the picture). Here we ensure exactly one "/" regardless
+       // of whether the given APIHost or path has a superfluous one.
+       return scheme + "://" + strings.TrimSuffix(c.APIHost, "/") + "/" + strings.TrimPrefix(path, "/")
 }
 
 // DiscoveryDocument is the Arvados server's description of itself.