git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Sean McAllister <smcallis@google.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Masaya Suzuki <masayasuzuki@google.com>,
	Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH v2 3/3] http: automatically retry some requests
Date: Thu, 15 Oct 2020 12:23:43 -0400	[thread overview]
Message-ID: <20201015162343.GC1104947@coredump.intra.peff.net> (raw)
In-Reply-To: <CAM4o00eefXK2CJ_FxwwVPpBKL01JsJANf+SdjCtw_0NVV82L+Q@mail.gmail.com>

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

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

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

  reply	other threads:[~2020-10-15 16:23 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-13 19:17 [PATCH v2 1/3] remote-curl: add testing for intelligent retry for HTTP Sean McAllister
2020-10-13 19:17 ` [PATCH v2 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA Sean McAllister
2020-10-13 20:46   ` Junio C Hamano
2020-10-13 20:58     ` Jeff King
2020-10-13 21:16       ` Daniel Stenberg
2020-10-14 14:57         ` Jeff King
2020-10-14 14:29     ` Sean McAllister
2020-10-14 17:11     ` Sean McAllister
2020-10-13 19:17 ` [PATCH v2 3/3] http: automatically retry some requests Sean McAllister
2020-10-13 21:14   ` Junio C Hamano
2020-10-14 14:28     ` Sean McAllister
2020-10-14 15:25       ` Junio C Hamano
2020-10-13 21:14   ` Jeff King
2020-10-13 23:45     ` brian m. carlson
2020-10-14 15:17       ` Jeff King
2020-10-14 19:09     ` Sean McAllister
2020-10-14 19:10       ` Sean McAllister
2020-10-14 19:34         ` Jeff King
2020-10-14 22:38           ` Sean McAllister
2020-10-15  0:04             ` Jonathan Nieder
2020-10-15 16:14               ` Jeff King
2020-10-15 16:24               ` Junio C Hamano
2020-10-14 19:55   ` Jeff King
2020-10-14 22:46     ` Sean McAllister
2020-10-15 16:23       ` Jeff King [this message]
2020-10-15 16:33         ` Sean McAllister
2020-10-13 20:40 ` [PATCH v2 1/3] remote-curl: add testing for intelligent retry for HTTP Junio C Hamano
2020-10-14 16:56   ` Sean McAllister
2020-10-14 18:13     ` Junio C Hamano
2020-10-14 18:21       ` Sean McAllister

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201015162343.GC1104947@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=masayasuzuki@google.com \
    --cc=smcallis@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).