On Tue, Oct 13, 2020 at 3:14 PM Jeff King <peff@xxxxxxxx> wrote:
>
> On Tue, Oct 13, 2020 at 01:17:29PM -0600, Sean McAllister wrote:
>
> > Exiting immediately becomes irksome when pulling large multi-repo code
> > bases such as Android or Chromium, as often the entire fetch operation
> > has to be restarted from the beginning due to an error in one repo. If
> > we can reduce how often that occurs, then it's a big win.
>
> I had hoped that libcurl might have some retry mechanisms, since the
> curl command-line tool has several --retry-* options. But it looks like
> that is all only at the tool level, and the library code doesn't know
> anything about it. So we are stuck driving the process ourselves.
>
> I do think you could be leveraging CURLINFO_RETRY_AFTER rather than
> implementing your own header parsing, though.
>
Ah I didn't know about CURLINFO_RETRY_AFTER, I'll look at that and use
it if I can.
> > static int http_request(const char *url,
> > void *result, int target,
> > const struct http_get_options *options)
> > {
>
> It looks like you trigger retries only from this function. But this
> doesn't cover all http requests that Git makes. That might be sufficient
> for your purposes (I think it would catch all of the initial contact),
> but it might not (it probably doesn't cover subsequent POSTs for fetch
> negotiation nor pack push; likewise I'm not sure if it covers much of
> anything after v2 stateless-connect is established).
>
You're right that I only trigger from this function. I've since removed them
in response to feedback on having too many tests, but I originally tested this
with:
t5539-fetch-http-shallow.sh
t5540-http-push-webdav.sh
t5541-http-push-smart.sh
t5550-http-fetch-dumb.sh
t5551-http-fetch-smart.sh
t5601-clone.sh
I'd have to look at the packet logs to see exactly what each of those
protocols is doing, but it seemed to cover _most_ of what they were doing.
Definitely open to adding retries in other places though.
> > struct active_request_slot *slot;
> > struct slot_results results;
> > - struct curl_slist *headers = http_copy_default_headers();
> > + struct curl_slist *headers;
>
> So here we stop copying the headers at the top of the function...
>
> > [...]
> > +retry:
> > [...]
> > + headers = http_copy_default_headers();
> > if (accept_language)
> > headers = curl_slist_append(headers, accept_language);
>
> And instead set them up totally here. Which make some sense, because we
> wouldn't want to append accept_language over and over. But who frees the
> old ones? There is a call to curl_slist_free_all(headers) later in the
> function, but it's after your "goto retry". So I think each retry would
> leak another copy of the list.
>
> The ideal thing would probably be to create the header list once, and
> then use it for each retry. That would require reordering some of the
> setup. If that's too much, then it would be OK to just create a new list
> from scratch on each call. Though in the latter case I suspect it may be
> simpler to wrap the whole function, like:
>
> static int http_request(...)
> {
> int try;
> int result;
> for (try = 0; try < max_retries; i++) {
> result = http_request_try(...);
> if (...result is not retryable...)
> break;
> }
> return result;
> }
>
> and then we'd know that the single-try function just needs to be
> self-contained, without worrying about gotos jumping around in it.
>
> -Peff
I like this idea, I've refactored it to do just this.
