Re: [PATCH 4/4] remote-curl: retry transient HTTP errors in post_rpc()

[Junio C Hamano <gitster@pobox.com>]

"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/http.c b/http.c
> index 950e2bbffa..36b559e596 100644
> --- a/http.c
> +++ b/http.c
> @@ -2476,6 +2476,34 @@ static long handle_rate_limit_retry(long slot_retry_after, int attempt)
>  	}
>  }
>  
> +int http_retry_wait(long http_code, long slot_retry_after, int attempt)
> +{
> +	long retry_delay;
> +
> +	if (http_max_retries <= 0)
> +		return -1;
> +	if (attempt > http_max_retries) {
> +		trace2_data_string("http", the_repository,
> +				   "http/429-error", "retries-exhausted");
> +		return -1;
> +	}
> +	retry_delay = handle_rate_limit_retry(slot_retry_after, attempt);
> +	if (retry_delay < 0)
> +		return -1;
> +	if (retry_delay > 0) {
> +		if (http_code == 429)
> +			warning(_("rate limited, waiting %ld seconds before retry"),
> +				retry_delay);
> +		else
> +			warning(_("HTTP %ld, waiting %ld seconds before retry"),
> +				http_code, retry_delay);
> +		trace2_data_intmax("http", the_repository,
> +				   "http/retry-sleep-seconds", retry_delay);
> +		sleep(retry_delay);
> +	}
> +	return 0;
> +}

This code looks familiar.  Beautifully extracted from ...

>  static int http_request_recoverable(const char *url,
>  			       void *result, int target,
>  			       struct http_get_options *options)
> @@ -2483,7 +2511,6 @@ static int http_request_recoverable(const char *url,
>  	static struct http_get_options empty_opts;
>  	int i = 3;
>  	int ret;
> -	int rate_limit_retries = http_max_retries;
>  	int attempt = 0;
>  
>  	if (!options)
> @@ -2506,9 +2533,8 @@ static int http_request_recoverable(const char *url,
>  	}
>  
>  	while ((ret == HTTP_REAUTH && --i) ||
> -	       (ret == HTTP_RATE_LIMITED && http_max_retries > 0 &&
> -		rate_limit_retries-- > 0)) {
> -		long retry_delay = -1;
> +	       (ret == HTTP_RATE_LIMITED &&
> +		!http_retry_wait(options->http_code, options->retry_after, ++attempt))) {
>  		/*
>  		 * The previous request may have put cruft into our output stream; we
>  		 * should clear it out before making our next request.
> @@ -2533,35 +2559,15 @@ static int http_request_recoverable(const char *url,
>  		default:
>  			BUG("Unknown http_request target");
>  		}
> -		if (ret == HTTP_RATE_LIMITED) {
> -			retry_delay = handle_rate_limit_retry(options->retry_after, ++attempt);
> -			if (retry_delay < 0)
> -				return HTTP_ERROR;
> -
> -			if (retry_delay > 0) {
> -				if (options->http_code == 429)
> -					warning(_("rate limited, waiting %ld seconds before retry"),
> -						retry_delay);
> -				else
> -					warning(_("HTTP %ld, waiting %ld seconds before retry"),
> -						options->http_code, retry_delay);
> -				trace2_data_intmax("http", the_repository,
> -						   "http/retry-sleep-seconds", retry_delay);
> -				sleep(retry_delay);
> -			}
> -		} else if (ret == HTTP_REAUTH) {
> +		if (ret == HTTP_REAUTH)
>  			http_reauth_prepare(1);
> -		}

... existing code around here.  It always feels good to see that an
introduction of a new function is accompanied by replacing many
lines of existsing code with a call to the new function.

Thanks.