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

From: Jeff King <peff@peff.net>
Date: Oct 15, 2020, 16:23 UTC
Message-ID: <20201015162343.GC1104947@coredump.intra.peff.net>
In-Reply-To: <CAM4o00eefXK2CJ_FxwwVPpBKL01JsJANf+SdjCtw_0NVV82L+Q@mail.gmail.com>

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

Show 7 quoted lines

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

Show 14 quoted lines

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