From f27f40fe44d4e0eec77d51d1685fd9df5e1b050b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 26 May 2025 23:23:23 +0200 Subject: [PATCH] fixup! gvfs-helper: create tool to fetch objects using the GVFS Protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some jobs of Git's CI/PR workflow run in Ubuntu 20.04. It would appear that that Ubuntu's version of libcurl, 7.68.0-1ubuntu2.25, behaves in an unexpected way: when running with `CURLOPT_FAILONERROR` enabled (which Git sets in `get_active_slot()`), the HTTP headers are not parsed in the failure code path. This has ramifications: `gvfs-helper` _wants_ to parse the `Retry-After` header, among other things, when encountering, say, a 503 (which is precisely what t5799.29 (integration: implicit-get: http_503: diff 2 commits) tests for, and likewise t5799.30 (integration: implicit-get: cache_http_503,no-fallback: diff 2 commits). If this `Retry-After` header is parsed as expected, in both of these test cases the `gvfs-helper` (which is called implicitly by `git diff` to retrieve the prerequisite Git objects) will back off six times, each time waiting 2 seconds (and printing "Waiting on hard throttle (sec): 100% (2/2), done." if run interactively), then try the thing once more, and then fail as expected. If this `Retry-After` header is _not_ parsed, `gvfs-helper` will take a _different_ failure code path. The symptom is the error message "Waiting to retry after network error (sec): 100% (8/8)", waiting 8 seconds, followed by several similar error messages that double the waiting time until 256 seconds, then dropping back to 8 seconds, doubling several times until 128 seconds, and then giving up. The end result looks like the expected failure, but it didn't take a mere 2x24 seconds in total to fail for good, but 2x~12½ minutes! As a consequence, the t5799 script alone regularly wasted over an hour in microsoft/git's CI jobs running on Ubuntu 20.04. The reason for this is https://github.com/curl/curl/commit/ab525c059eb (http: have CURLOPT_FAILONERROR fail after all headers, 2021-01-04) which is curl-7_75_0~136, i.e. not part of libcurl 7.68.0-1ubuntu2.25. This change allows libcurl to parse the HTTP headers even in the failure code path, even when `CURLOPT_FAILONERROR` is enabled. It would be tempting to remedy this problem here in `gvfs-helper` by disabling `CURLOPT_FAILONERROR`; The code in `gvfs-helper` seems already well-prepared to handle all of the errors based on the HTTP result code, even without libcurl returning `CURLE_HTTP_RETURNED_ERROR`. However, in my tests, doing this unilaterally results in occasional (read: flaky) 501s in t5799.25(HTTP POST Auth on Origin Server). So let's do this only when the cURL version is really old (thanks, Debian!). This results in a slight change of behavior: When authentication fails, the connection is now kept open. Testament to this behavioral change is t5799.24(HTTP GET Auth on Origin Server) which wanted to verify that there are two connections when authentication is required, but now there is only one (the rest of the test case verifies that fetching the loose object worked, i.e. that the authentication succeeded). Let's just skip that verification in case of an older cURL version. I briefly considered an alternative, where the affected test cases of t5799 would be skipped depending on this prerequisite: test_lazy_prereq CURL_7_75_OR_NEWER ' test 7.75.0 = "$(printf '%s\n' 7.75.0 $(curl --version | sed -n '1s/^curl \([^ ]*\).*/\1/p') | sort -n -t . | head -n 1)" ' But that is not only one ugly prereq definition, it would also lose test coverage for Ubuntu 20.04 (and guarantee a bad experience of microsoft/git's users on that operating system in case of 503s). Since https://github.com/microsoft/git/blob/v2.49.0.vfs.0.3/.github/workflows/build-git-installers.yml#L559 explicitly targets Ubuntu 20.04, I rejected that alternative. Signed-off-by: Johannes Schindelin --- gvfs-helper.c | 13 +++++++++++++ t/t5799-gvfs-helper.sh | 12 +++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/gvfs-helper.c b/gvfs-helper.c index 1a06eb2395ffe4..f1f621e73f24cf 100644 --- a/gvfs-helper.c +++ b/gvfs-helper.c @@ -2913,6 +2913,19 @@ static void do_req(const char *url_base, curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0); /* not a HEAD request */ curl_easy_setopt(slot->curl, CURLOPT_URL, rest_url.buf); curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, params->headers); + if (curl_version_info(CURLVERSION_NOW)->version_num < 0x074b00) + /* + * cURL 7.75.0 allows headers to be parsed even when + * `CURLOPT_FAILONERROR` is enabled and the HTTP result code + * indicates an error. This is the behavior expected by + * `gvfs-helper`. + * + * On older cURL versions, `gvfs-helper` still needs to parse + * the HTTP headers and therefore needs to _not_ fail upon + * HTTP result codes indicating errors; For newer cURL + * versions, we still prefer to enable `FAILONERROR`. + */ + curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, (long)0); if (params->b_is_post) { curl_easy_setopt(slot->curl, CURLOPT_POST, 1); diff --git a/t/t5799-gvfs-helper.sh b/t/t5799-gvfs-helper.sh index 5a118e522c8b07..6d7a52f9160840 100755 --- a/t/t5799-gvfs-helper.sh +++ b/t/t5799-gvfs-helper.sh @@ -1122,6 +1122,13 @@ test_expect_success 'http-error: 503 Service Unavailable (with retry and no-fall # ################################################################# +test_lazy_prereq CURL_7_75_OR_NEWER ' + case "$(curl version | sed -n "1s/^curl \([^ ]*\).*/\1/p")" in + ""|[0-6].*|7.[0-9]*.*|7.[1-6][0-9].*|7.7[0-4]*.*) return 1;; + *) return 0;; + esac +' + test_expect_success 'HTTP GET Auth on Origin Server' ' test_when_finished "per_test_cleanup" && start_gvfs_protocol_server_with_mayhem http_401 && @@ -1148,7 +1155,10 @@ test_expect_success 'HTTP GET Auth on Origin Server' ' test_cmp "$OID_ONE_BLOB_FILE" OUT.actual && verify_objects_in_shared_cache "$OID_ONE_BLOB_FILE" && - verify_connection_count 2 + if test_have_prereq CURL_7_75_OR_NEWER + then + verify_connection_count 2 + fi ' test_expect_success 'HTTP POST Auth on Origin Server' '