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

[Jeff King <peff@peff.net>]

On Tue, Oct 13, 2020 at 01:17:29PM -0600, Sean McAllister wrote:

> +/*
> + * 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.

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

>  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