git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] Retry HTTP requests on SSL connect failures

From
Jeff King <peff@peff.net>
Date
Oct 1, 2012, 22:18 UTC
Message-ID
<20121001221817.GA12496@sigill.intra.peff.net>
In-Reply-To
<1349126586-755-1-git-send-email-spearce@spearce.org>
On Mon, Oct 01, 2012 at 02:23:06PM -0700, Shawn O. Pearce wrote:
Show 17 quoted lines
> From: "Shawn O. Pearce" <spearce@spearce.org>
> 
> When libcurl fails to connect to an SSL server always retry the
> request once. Since the connection failed before the HTTP headers
> can be sent, no data has exchanged hands, so the remote side has
> not learned of the request and will not perform it twice.
> 
> In the wild we have seen git-remote-https fail to connect to
> some load-balanced SSL servers sporadically, while modern popular
> browsers (e.g. Firefox and Chromium) have no trouble with the same
> server pool.
> 
> Lets assume the site operators (Hi Google!) have a clue and are
> doing everything they already can to ensure secure, successful
> SSL connections from a wide range of HTTP clients. Implementing a
> single level of retry in the client can make it more robust against
> transient failure modes.

I find this a little distasteful just because we haven't figured out the actual _reason_ for the failure. That is, I'm not convinced this isn't something that curl or the ssl library can't handle internally if we would only configure them correctly. Did you ever follow up on tweaking the session caching options for curl?

Have you tried running your fails-after-a-few-hours request with other clients that don't have the problem and seeing what they do (I'm thinking a small webkit harness or something would be the most feasible)?

That being said, you did make it so that it only kicks in during ssl connect errors:

Show 11 quoted lines
> +	for (attempts = 0; attempts < 2; attempts++) {
> +		if (start_active_slot(slot)) {
> +			run_active_slot(slot);
> +			if (slot->results->curl_result == CURLE_SSL_CONNECT_ERROR)
> +				continue;
> +			ret = handle_curl_result(slot);
> +		} else {
> +			error("Unable to start HTTP request for %s", url);
> +			ret = HTTP_START_FAILED;
> +		}
> +		break;

which means it shouldn't really be affecting the general populace. So even though it feels like a dirty hack, at least it is self-contained, and it does fix a real-world problem. If your answer to the above questions is "hunting this further is just not worth the effort", I can live with that.

Show 11 quoted lines
> diff --git a/remote-curl.c b/remote-curl.c
> index a269608..04a379c 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -353,6 +353,8 @@ static int run_slot(struct active_request_slot *slot)
>  
>  	slot->results = &results;
>  	slot->curl_result = curl_easy_perform(slot->curl);
> +	if (slot->curl_result == CURLE_SSL_CONNECT_ERROR)
> +		slot->curl_result = curl_easy_perform(slot->curl);
>  	finish_active_slot(slot);
How come the first hunk gets a nice for-loop and this one doesn't?

Also, are these hunks the only two spots where this error can come up? The first one does http_request, which handles smart-http GET requests. the second does run_slot, which handles smart-http POST requests.

Some of the dumb http fetches will go through http_request. But some will not. And I think almost none of dumb http push will.

-Peff
Previous: Junio C HamanoNext: Shawn Pearce
Message 30 of 36 in “Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0”
  1. Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0Shawn O. Pearce, Sep 20, 2012
  2. Shawn PearceSep 20, 2012
  3. Jeff KingSep 20, 2012
  4. Jeff KingSep 20, 2012
  5. Shawn PearceSep 20, 2012
  6. Revert "retry request without query when info/refs?query fails"Shawn O. Pearce, Sep 20, 2012
  7. Junio C HamanoSep 20, 2012
  8. Junio C HamanoSep 20, 2012
  9. Jeff KingSep 20, 2012
  10. 0/2 smart http toggle switch fails"Jeff King, Sep 20, 2012
  11. 1/2 remote-curl: rename is_http variableJeff King, Sep 20, 2012
  12. 2/2 remote-curl: let users turn off smart httpJeff King, Sep 20, 2012
  13. Junio C HamanoSep 20, 2012
  14. Jeff KingSep 20, 2012
  15. Junio C HamanoSep 20, 2012
  16. Jeff KingSep 20, 2012
  17. Junio C HamanoSep 20, 2012
  18. Jeff KingSep 20, 2012
  19. Junio C HamanoSep 21, 2012
  20. Jeff KingSep 21, 2012
  21. Jeff KingSep 20, 2012
  22. Shawn PearceSep 20, 2012
  23. Jeff KingSep 21, 2012
  24. Shawn PearceSep 21, 2012
  25. Retry HTTP requests on SSL connect failuresShawn O. Pearce, Oct 1, 2012
  26. Junio C HamanoOct 1, 2012
  27. Junio C HamanoOct 1, 2012
  28. Jeff KingOct 1, 2012
  29. Junio C HamanoOct 1, 2012
  30. Jeff KingOct 1, 2012
  31. Shawn PearceOct 2, 2012
  32. Drew NorthupOct 2, 2012
  33. Drew NorthupOct 2, 2012
  34. Re* [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0Junio C Hamano, Sep 20, 2012
  35. 1/2 Disable dumb HTTP fallback with GIT_DUMB_HTTP_FALLBACK=falseJunio C Hamano, Sep 20, 2012
  36. 2/2 remote-curl: make dumb-http fallback configurable per URLJunio C Hamano, Sep 20, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.