Re: [PATCH v2 3/3] http: automatically retry some requests
- From
Jeff King <peff@peff.net>
- Date
- Oct 14, 2020, 19:55 UTC
- Message-ID
- <20201014195544.GA365911@coredump.intra.peff.net>
- In-Reply-To
- <20201013191729.2524700-3-smcallis@google.com>
On Tue, Oct 13, 2020 at 01:17:29PM -0600, Sean McAllister wrote:
Show 15 quoted lines
> +/*
> + * check for a retry-after header in the given headers string, if found, then
> + * honor it, otherwise do an exponential backoff up to the max on the current
> + * delay
> +*/
> +static int http_retry_after(const struct strbuf headers, int cur_delay_sec)
> +{
> + int delay_sec;
> + char *end;
> + char* value = http_header_value(headers, "retry-after");
> +
> + if (value) {
> + delay_sec = strtol(value, &end, 0);
> + free(value);
> + if (*value && *end == '\0' && delay_sec >= 0) {This looks at the contents of the just-freed "value" memory block.
Show 8 quoted lines
> + if (delay_sec > http_max_delay_sec) {
> + die(Q_("server requested retry after %d second,"
> + " which is longer than max allowed\n",
> + "server requested retry after %d seconds,"
> + " which is longer than max allowed\n", delay_sec),
> + delay_sec);
> + }
> + return delay_sec;I guess there's no point in being gentle here. We could shrink the retry time to our maximum allowed, but the server just told us not to bother. But would this die() mask the actual http error we encountered, which is surely the more interesting thing for the user?
I wonder if it needs to be returning a "do not bother retrying" value, which presumably would cause the caller to propagate the real failure in the usual way.
Show 10 quoted lines
> static int http_request(const char *url,
> void *result, int target,
> const struct http_get_options *options)
> {
> struct active_request_slot *slot;
> struct slot_results results;
> - struct curl_slist *headers = http_copy_default_headers();
> + struct curl_slist *headers;
> struct strbuf buf = STRBUF_INIT;
> + struct strbuf result_headers = STRBUF_INIT;This new result_headers strbuf is filled in for every request, but I don't think us ever releasing it (whether we retry or not). So I think it's leaking for each request.
It sounds like you're going to rework this to put the retry loop outside of http_request(), so it may naturally get fixed there. But I thought it worth mentioning.
> + curl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, &result_headers); > + curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_buffer);
After looking at your parsing code, I wondered if there was a way to just get a single header out of curl. But according to the documentation for CURLOPT_HEADERFUNCTION, it will pass back individual lines anyway. Perhaps it would be simpler to have the callback function understand that we only care about getting "Retry-After".
The documentation says it doesn't support header folding, but that's probably OK for our purposes. It's deprecated, and your custom parsing doesn't handle it either. :) And most importantly, we won't misbehave terribly if we see it in the wild (we'll just ignore that header).
-Peff