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

[Jonathan Nieder <jrnieder@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:

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