Re: [PATCH v2 3/3] http: automatically retry some requests

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 15, 2020, 00:04 UTC
Message-ID: <20201015000410.GB328643@google.com>
In-Reply-To: <CAM4o00eZjr2apH6WO-sTvuOfR-cuiSh1yhh3j=14ZFstXDz7bA@mail.gmail.com>

Hi,

Sean McAllister wrote:

> On Wed, Oct 14, 2020 at 1:34 PM Jeff King <peff@peff.net> wrote:
>> On Wed, Oct 14, 2020 at 01:10:46PM -0600, Sean McAllister wrote:

Show 19 quoted lines

>>> I took a look, it looks like CURLINFO_RETRY_AFTER was only added in
>>> 7.66 (September, 2019),  so
>>> I don't think it's reasonable to rely on it for getting the
>>> Retry-After value in this case.
>>
>> I agree that's pretty recent.
>>
>> How important is it that we respect it? I.e., we'd have some sane retry
>> behavior if the header is missing anyway. On older curl versions, how
>> bad would it be to just use that fallback behavior all the time?
>
> Some large projects (Android, Chrome), use git with a distributed
> backend to feed changes to automated builders and such.  We can
> actually get into a case where DDOS mitigation kicks in and 429s start
> going out.  In that case I think it's pretty important that we honor
> the Retry-After field so we're good citizens and whoever's running the
> backend service has some options for traffic shaping to manage load.
> In general you're right it doesn't matter _that_ much but in at least
> the specific case I have in my head, it does.

I see. With Peff's proposal, the backend service could still set Retry-After, and *modern* machines with new enough libcurl would still respect it, but older machines[*] would have to use some fallback behavior.

I suppose that fallback behavior could be to not retry at all. That sounds appealing to me since it would be more maintainable (no custom header parsing) and the fallback would be decreasingly relevant over time as users upgrade to modern versions of libcurl and git. What do you think?

Thanks, Jonathan

[*] This represents two cases: if CURLINFO_RETRY_AFTER is not defined, meaning that libcurl-dev was too old at *build time*, and if getinfo returned CURLE_UNKNOWN_OPTION, meaning that libcurl was too old at run time.

You might wonder how we handle the too-old-at-build-time case. The usual trick is to provide a helper function with a stub implementation:

 #ifndef CURLINFO_RETRY_AFTER
 static inline CURLcode retry_after(CURL *handle, curl_off_t *retry)
 {
 	return CURLE_UNKNOWN_OPTION;
 }
 #else
 static inline CURLcode retry_after(CURL *handle, curl_off_t *retry)
 {
 	return curl_easy_getinfo(handle, CURLINFO_RETRY_AFTER, retry);
 }
 #endif