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

On Wed, Oct 14, 2020 at 04:46:57PM -0600, Sean McAllister wrote:

quoted

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.

I've moved this check up a couple levels in v3, so that if we get too large
a retry value, then we'll print this message as a warning and quit retrying,
which will unmask the underlying HTTP error.

Thanks, that sounds much better.

quoted

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

I'll put this in my todo pile to think on a little, it'd be nice not
to have expand the strbuf with every request, but also not a huge
overhead.

I was less concerned with the overhead of the strbuf (http requests are
pretty heavyweight already) and more that it could simplify your parsing
if you could just do it left-to-right on a single line:

	char *line = xmemdupz(buffer, size);
	const char *p = line;
	if (skip_iprefix(p, "retry-after:", &p)) {
		char *end;
		while (isspace(*p))
			p++;
		opts->retry_after = strtol(p, &end, 10);
		/* if you want to be pedantic */
		if (*end && *end != '\r' && *end != '\n')
			opts->retry_after = 0; /* warn, too? */
	}

If you want to be clever, you could probably avoid the extra allocation,
but I think being able to parse with simple string functions makes it
much more obvious that we don't walk off the end of the input.

-Peff