Skip to content

Commit 457a019

Browse files
committed
fixup! gvfs-helper: create tool to fetch objects using the GVFS Protocol
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 curl/curl@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 at compile time 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 <[email protected]>
1 parent 1030eb5 commit 457a019

File tree

3 files changed

+23
-1
lines changed

3 files changed

+23
-1
lines changed

git-curl-compat.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,4 +53,13 @@
5353
#define GIT_CURL_HAVE_CURLSSLOPT_AUTO_CLIENT_CERT
5454
#endif
5555

56+
/**
57+
* 7.75.0 allows headers to be parsed even when `CURLOPT_FAILONERROR` is
58+
* enabled and the HTTP result code indicates an error. This is the behavior
59+
* expected by `gvfs-helper`.
60+
*/
61+
#if LIBCURL_VERSION_NUM >= 0x074b00
62+
#define GIT_CURL_HEADERS_ARE_ALWAYS_PARSED_EVEN_WITH_FAILONERROR
63+
#endif
64+
5665
#endif

gvfs-helper.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2913,6 +2913,9 @@ static void do_req(const char *url_base,
29132913
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0); /* not a HEAD request */
29142914
curl_easy_setopt(slot->curl, CURLOPT_URL, rest_url.buf);
29152915
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, params->headers);
2916+
#ifndef GIT_CURL_HEADERS_ARE_ALWAYS_PARSED_EVEN_WITH_FAILONERROR
2917+
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, (long)0);
2918+
#endif
29162919

29172920
if (params->b_is_post) {
29182921
curl_easy_setopt(slot->curl, CURLOPT_POST, 1);

t/t5799-gvfs-helper.sh

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1122,6 +1122,13 @@ test_expect_success 'http-error: 503 Service Unavailable (with retry and no-fall
11221122
#
11231123
#################################################################
11241124

1125+
test_lazy_prereq CURL_7_75_OR_NEWER '
1126+
case "$(git version --build-options | sed -n "s/^libcurl: //p")" in
1127+
[0-6].*|7.[0-9]*.*|7.[1-6][0-9].*|7.7[0-4]*.*) return 1;;
1128+
*) return 0;;
1129+
esac
1130+
'
1131+
11251132
test_expect_success 'HTTP GET Auth on Origin Server' '
11261133
test_when_finished "per_test_cleanup" &&
11271134
start_gvfs_protocol_server_with_mayhem http_401 &&
@@ -1148,7 +1155,10 @@ test_expect_success 'HTTP GET Auth on Origin Server' '
11481155
test_cmp "$OID_ONE_BLOB_FILE" OUT.actual &&
11491156
11501157
verify_objects_in_shared_cache "$OID_ONE_BLOB_FILE" &&
1151-
verify_connection_count 2
1158+
if test_have_prereq CURL_7_75_OR_NEWER
1159+
then
1160+
verify_connection_count 2
1161+
fi
11521162
'
11531163

11541164
test_expect_success 'HTTP POST Auth on Origin Server' '

0 commit comments

Comments
 (0)