{"thread":{"id":"54409","subject":"[PATCH v2 1/3] remote-curl: add testing for intelligent retry for HTTP","startedAt":"2020-10-13T19:17:44Z","lastAt":"2020-10-15T16:34:10Z","messageCount":30,"participants":["Sean McAllister","Junio C Hamano","Jeff King","brian m. carlson","Daniel Stenberg","Jonathan Nieder"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"407474","messageId":"20201013191729.2524700-1-smcallis@google.com","threadId":"54409","inReplyTo":null,"subject":"[PATCH v2 1/3] remote-curl: add testing for intelligent retry for HTTP","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-13T19:17:27Z","receivedAt":"2020-10-13T19:17:44Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"HTTP servers can sometimes throw errors, but that doesn't mean we should\ngive up. If we have an error condition that we can reasonably retry on,\nthen we should.\n\nThis change is tricky because it requires a new CGI script to test as we\nneed to be able to instruct the server to throw an error(s) before\nsucceeding. We do this by encoding an error code and optional value for\nRetry-After into the URL, followed by the real endpoint:\n\n /error_ntime/dc724af1//429/10/smart/server\n\nThis is a bit hacky, but really the best we can do since HTTP is\nfundamentally stateless. The URL is uniquefied by a nonce and we leave\na breadcrumb on disk so all accesses after the first redirect to the\nappropriate endpoint.\n\nSigned-off-by: Sean McAllister \n---\n t/lib-httpd.sh | 8 ++++\n t/lib-httpd/apache.conf | 9 +++++\n t/lib-httpd/error-ntime.sh | 80 ++++++++++++++++++++++++++++++++++++++\n t/t5601-clone.sh | 9 +++++\n 4 files changed, 106 insertions(+)\n create mode 100755 t/lib-httpd/error-ntime.sh\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex d2edfa4c50..edc10b6f4b 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -132,6 +132,7 @@ prepare_httpd() {\n \tinstall_script incomplete-length-upload-pack-v2-http.sh\n \tinstall_script incomplete-body-upload-pack-v2-http.sh\n \tinstall_script broken-smart-http.sh\n+\tinstall_script error-ntime.sh\n \tinstall_script error-smart-http.sh\n \tinstall_script error.sh\n \tinstall_script apply-one-time-perl.sh\n@@ -308,3 +309,10 @@ check_access_log() {\n \t\ttest_cmp \"$1\" access.log.stripped\n \tfi\n }\n+\n+# generate a process unique one-up value\n+global_counter_for_nonce=0\n+gen_nonce () {\n+\tglobal_counter_for_nonce=$((global_counter_for_nonce + 1)) &&\n+\techo \"$global_counter_for_nonce\"\n+}\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex afa91e38b0..77c495e164 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -117,6 +117,12 @@ Alias /auth/dumb/ www/auth/dumb/\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\n \n+\n+# This may be suffixed with any path for redirection, so it should come before\n+# any of the other aliases, particularly the /smart_*[^/]*/(.*) alias as that can\n+# match a lot of URLs\n+ScriptAlias /error_ntime/ error-ntime.sh/\n+\n ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/\n ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/\n ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/\n@@ -137,6 +143,9 @@ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1\n \n \tOptions ExecCGI\n \n+\n+\tOptions ExecCGI\n+\n \n \tOptions ExecCGI\n \ndiff --git a/t/lib-httpd/error-ntime.sh b/t/lib-httpd/error-ntime.sh\nnew file mode 100755\nindex 0000000000..64dc878746\n--- /dev/null\n+++ b/t/lib-httpd/error-ntime.sh\n@@ -0,0 +1,80 @@\n+#!/bin/sh\n+\n+# Script to simulate a transient error code with Retry-After header set.\n+#\n+# PATH_INFO must be of the form /////\n+# (eg: /dc724af1/3/429/10/some/url)\n+#\n+# The value uniquely identifies the URL, since we're simulating\n+# a stateful operation using a stateless protocol, we need a way to \"namespace\"\n+# URLs so that they don't step on each other.\n+#\n+# The first times this endpoint is called, it will return the given\n+# , and if the is non-negative, it will set the\n+# Retry-After head to that value.\n+#\n+# Subsequent calls will return a 302 redirect to .\n+#\n+# Supported error codes are 429,502,503, and 504\n+\n+print_status() {\n+\tif [ \"$1\" -eq \"302\" ]; then\n+\t\tprintf \"Status: 302 Found\\n\"\n+\telif [ \"$1\" -eq \"429\" ]; then\n+\t\tprintf \"Status: 429 Too Many Requests\\n\"\n+\telif [ \"$1\" -eq \"502\" ]; then\n+\t\tprintf \"Status: 502 Bad Gateway\\n\"\n+\telif [ \"$1\" -eq \"503\" ]; then\n+\t\tprintf \"Status: 503 Service Unavailable\\n\"\n+\telif [ \"$1\" -eq \"504\" ]; then\n+\t\tprintf \"Status: 504 Gateway Timeout\\n\"\n+\telse\n+\t\tprintf \"Status: 500 Internal Server Error\\n\"\n+\tfi\n+\tprintf \"Content-Type: text/plain\\n\"\n+}\n+\n+# set $@ to components of PATH_INFO\n+IFS='/'\n+set -f\n+set -- $PATH_INFO\n+set +f\n+\n+# pull out first four path components\n+shift\n+nonce=$1\n+times=$2\n+code=$3\n+retry=$4\n+shift 4\n+\n+# glue the rest back together as redirect path\n+path=\"\"\n+while [ \"$#\" -gt \"0\" ]; do\n+\tpath=\"${path}/$1\"\n+\tshift\n+done\n+\n+# leave a cookie for this request/retry count\n+state_file=\"request_${REMOTE_ADDR}_${nonce}_${times}_${code}_${retry}\"\n+\n+if [ ! -f \"$state_file\" ]; then\n+\techo 0 > \"$state_file\"\n+fi\n+\n+read -r cnt < \"$state_file\"\n+if [ \"$cnt\" -lt \"$times\" ]; then\n+\techo $((cnt+1)) > \"$state_file\"\n+\n+\t# return error\n+\tprint_status \"$code\"\n+\tif [ \"$retry\" -ge \"0\" ]; then\n+\t\tprintf \"Retry-After: %s\\n\" \"$retry\"\n+\tfi\n+else\n+\t# redirect\n+\tprint_status 302\n+\tprintf \"Location: %s?%s\\n\" \"$path\" \"${QUERY_STRING}\"\n+fi\n+\n+echo\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 7df3c5373a..72aaed5a93 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -756,6 +756,15 @@ test_expect_success 'partial clone using HTTP' '\n \tpartial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/smart/server\"\n '\n \n+test_expect_success 'partial clone using HTTP with redirect' '\n+\t_NONCE=`gen_nonce` && export _NONCE &&\n+\tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n+\tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n+\tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n+\tpartial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\"\n+'\n+\n+\n # DO NOT add non-httpd-specific tests here, because the last part of this\n # test script is only executed when httpd is available and enabled.\n \n-- \n2.28.0.1011.ga647a8990f-goog\n\n"},{"id":"407475","messageId":"20201013191729.2524700-2-smcallis@google.com","threadId":"54409","inReplyTo":"20201013191729.2524700-1-smcallis@google.com","subject":"[PATCH v2 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-13T19:17:28Z","receivedAt":"2020-10-13T19:17:46Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"CURLOPT_FILE has been deprecated since 2003.\n\nSigned-off-by: Sean McAllister \n---\n http-push.c | 6 +++---\n http-walker.c | 2 +-\n http.c | 6 +++---\n remote-curl.c | 4 ++--\n 4 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 6a4a43e07f..2e6fee3305 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -894,7 +894,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n \tslot->results = &results;\n \tcurl_setup_http(slot->curl, url, DAV_LOCK, &out_buffer, fwrite_buffer);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &in_buffer);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);\n \n \tlock = xcalloc(1, sizeof(*lock));\n \tlock->timeout = -1;\n@@ -1151,7 +1151,7 @@ static void remote_ls(const char *path, int flags,\n \tcurl_setup_http(slot->curl, url, DAV_PROPFIND,\n \t\t\t&out_buffer, fwrite_buffer);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &in_buffer);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);\n \n \tif (start_active_slot(slot)) {\n \t\trun_active_slot(slot);\n@@ -1225,7 +1225,7 @@ static int locking_available(void)\n \tcurl_setup_http(slot->curl, repo->url, DAV_PROPFIND,\n \t\t\t&out_buffer, fwrite_buffer);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &in_buffer);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);\n \n \tif (start_active_slot(slot)) {\n \t\trun_active_slot(slot);\ndiff --git a/http-walker.c b/http-walker.c\nindex 4fb1235cd4..6c630711d1 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -384,7 +384,7 @@ static void fetch_alternates(struct walker *walker, const char *base)\n \talt_req.walker = walker;\n \tslot->callback_data = &alt_req;\n \n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &buffer);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);\n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, url.buf);\n \ndiff --git a/http.c b/http.c\nindex 8b23a546af..b3c1669388 100644\n--- a/http.c\n+++ b/http.c\n@@ -1921,7 +1921,7 @@ static int http_request(const char *url,\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);\n \t} else {\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, result);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, result);\n \n \t\tif (target == HTTP_REQUEST_FILE) {\n \t\t\toff_t posn = ftello(result);\n@@ -2337,7 +2337,7 @@ struct http_pack_request *new_direct_http_pack_request(\n \t}\n \n \tpreq->slot = get_active_slot();\n-\tcurl_easy_setopt(preq->slot->curl, CURLOPT_FILE, preq->packfile);\n+\tcurl_easy_setopt(preq->slot->curl, CURLOPT_WRITEDATA, preq->packfile);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER,\n@@ -2508,7 +2508,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \n \tfreq->slot = get_active_slot();\n \n-\tcurl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);\n+\tcurl_easy_setopt(freq->slot->curl, CURLOPT_WRITEDATA, freq);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_FAILONERROR, 0);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 32cc4a0c55..7f44fa30fe 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -847,7 +847,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &buf);\n \n \terr = run_slot(slot, results);\n \n@@ -1012,7 +1012,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece\n \trpc_in_data.slot = slot;\n \trpc_in_data.check_pktline = stateless_connect;\n \tmemset(&rpc_in_data.pktline_state, 0, sizeof(rpc_in_data.pktline_state));\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &rpc_in_data);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n \n-- \n2.28.0.1011.ga647a8990f-goog\n\n"},{"id":"407476","messageId":"20201013191729.2524700-3-smcallis@google.com","threadId":"54409","inReplyTo":"20201013191729.2524700-1-smcallis@google.com","subject":"[PATCH v2 3/3] http: automatically retry some requests","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-13T19:17:29Z","receivedAt":"2020-10-13T19:17:47Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"Some HTTP response codes indicate a server state that can support\nretrying the request rather than immediately erroring out. The server\ncan also provide information about how long to wait before retries to\nvia the Retry-After header. So check the server response and retry\nsome reasonable number of times before erroring out to better accomodate\ntransient errors.\n\nExiting immediately becomes irksome when pulling large multi-repo code\nbases such as Android or Chromium, as often the entire fetch operation\nhas to be restarted from the beginning due to an error in one repo. If\nwe can reduce how often that occurs, then it's a big win.\n\nSigned-off-by: Sean McAllister \n---\n Documentation/config/http.txt | 5 ++\n http.c | 124 +++++++++++++++++++++++++++++++++-\n http.h | 2 +-\n remote-curl.c | 2 +-\n t/t5601-clone.sh | 6 +-\n 5 files changed, 133 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/http.txt b/Documentation/config/http.txt\nindex 3968fbb697..5d5baef967 100644\n--- a/Documentation/config/http.txt\n+++ b/Documentation/config/http.txt\n@@ -260,6 +260,11 @@ http.followRedirects::\n \tthe base for the follow-up requests, this is generally\n \tsufficient. The default is `initial`.\n \n+http.retryLimit::\n+\tSome HTTP status codes (eg: 429, 503) can reasonably be retried if\n+\tthey're encountered. This value configures the number of retry attempts\n+\tbefore giving up. The default retry limit is 3.\n+\n http..*::\n \tAny of the http.* options above can be applied selectively to some URLs.\n \tFor a config key to match a URL, each element of the config key is\ndiff --git a/http.c b/http.c\nindex b3c1669388..f0147582f9 100644\n--- a/http.c\n+++ b/http.c\n@@ -92,6 +92,9 @@ static const char *http_proxy_ssl_key;\n static const char *http_proxy_ssl_ca_info;\n static struct credential proxy_cert_auth = CREDENTIAL_INIT;\n static int proxy_ssl_cert_password_required;\n+static int http_retry_limit = 3;\n+static int http_default_delay_sec = 2;\n+static int http_max_delay_sec = 60;\n \n static struct {\n \tconst char *name;\n@@ -219,6 +222,56 @@ size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)\n \treturn nmemb;\n }\n \n+\n+/* return 1 for a retryable HTTP code, 0 otherwise */\n+static int retryable_code(int code)\n+{\n+\tswitch(code) {\n+\tcase 429: /* fallthrough */\n+\tcase 502: /* fallthrough */\n+\tcase 503: /* fallthrough */\n+\tcase 504: return 1;\n+\tdefault: return 0;\n+\t}\n+}\n+\n+/*\n+ * Query the value of an HTTP header.\n+ *\n+ * If the header is found, then a newly allocate string containing the value\n+ * is returned.\n+ *\n+ * If not found, returns NULL\n+ */\n+static char* http_header_value(const struct strbuf headers, const char *header)\n+{\n+\tconst size_t header_len = strlen(header);\n+\tconst char* beg = headers.buf, *end;\n+\tconst char* ptr = strcasestr(beg, header), *eol;\n+\n+\twhile (ptr) {\n+\t\t/* headers should have no leading whitespace, and end with ':' */\n+\t\tend = ptr + header_len;\n+\t\tif ((ptr != beg && ptr[-1] != '\\n') || *end != ':') {\n+\t\t\tptr = strcasestr(end, header);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/* skip leading LWS */\n+\t\tptr = end + 1;\n+\t\twhile (*ptr && isspace(*ptr) && *ptr != '\\r')\n+\t\t\tptr++;\n+\n+\t\t/* skip trailing LWS */\n+\t\teol = strchrnul(ptr, '\\r');\n+\t\twhile (eol > ptr && isspace(eol[-1]))\n+\t\t\teol--;\n+\n+\t\treturn xstrndup(ptr, eol-ptr);\n+\t}\n+\treturn NULL;\n+}\n+\n static void closedown_active_slot(struct active_request_slot *slot)\n {\n \tactive_requests--;\n@@ -452,6 +505,11 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(\"http.retrylimit\", var)) {\n+\t\thttp_retry_limit = git_config_int(var, value);\n+\t\treturn 0;\n+\t}\n+\n \t/* Fall back on the default ones */\n \treturn git_default_config(var, value, cb);\n }\n@@ -1668,7 +1726,7 @@ static int handle_curl_result(struct slot_results *results)\n }\n \n int run_one_slot(struct active_request_slot *slot,\n-\t\t struct slot_results *results)\n+\t\t struct slot_results *results, int *http_code)\n {\n \tslot->results = results;\n \tif (!start_active_slot(slot)) {\n@@ -1678,6 +1736,8 @@ int run_one_slot(struct active_request_slot *slot,\n \t}\n \n \trun_active_slot(slot);\n+\tif (http_code)\n+\t\t*http_code = results->http_code;\n \treturn handle_curl_result(results);\n }\n \n@@ -1903,20 +1963,58 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n \n+/*\n+ * check for a retry-after header in the given headers string, if found, then\n+ * honor it, otherwise do an exponential backoff up to the max on the current\n+ * delay\n+*/\n+static int http_retry_after(const struct strbuf headers, int cur_delay_sec)\n+{\n+\tint delay_sec;\n+\tchar *end;\n+\tchar* value = http_header_value(headers, \"retry-after\");\n+\n+\tif (value) {\n+\t\tdelay_sec = strtol(value, &end, 0);\n+\t\tfree(value);\n+\t\tif (*value && *end == '\\0' && delay_sec >= 0) {\n+\t\t\tif (delay_sec > http_max_delay_sec) {\n+\t\t\t\tdie(Q_(\"server requested retry after %d second,\"\n+\t\t\t\t\t \" which is longer than max allowed\\n\",\n+\t\t\t\t\t \"server requested retry after %d seconds,\"\n+\t\t\t\t\t \" which is longer than max allowed\\n\", delay_sec),\n+\t\t\t\t\tdelay_sec);\n+\t\t\t}\n+\t\t\treturn delay_sec;\n+\t\t}\n+\t}\n+\n+\tcur_delay_sec *= 2;\n+\treturn cur_delay_sec >= http_max_delay_sec ? http_max_delay_sec : cur_delay_sec;\n+}\n+\n static int http_request(const char *url,\n \t\t\tvoid *result, int target,\n \t\t\tconst struct http_get_options *options)\n {\n \tstruct active_request_slot *slot;\n \tstruct slot_results results;\n-\tstruct curl_slist *headers = http_copy_default_headers();\n+\tstruct curl_slist *headers;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf result_headers = STRBUF_INIT;\n \tconst char *accept_language;\n \tint ret;\n+\tint retry_cnt = 0;\n+\tint retry_delay_sec = http_default_delay_sec;\n+\tint http_code;\n \n+retry:\n \tslot = get_active_slot();\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n \n+\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, &result_headers);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_buffer);\n+\n \tif (result == NULL) {\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);\n \t} else {\n@@ -1936,6 +2034,7 @@ static int http_request(const char *url,\n \n \taccept_language = get_accept_language();\n \n+\theaders = http_copy_default_headers();\n \tif (accept_language)\n \t\theaders = curl_slist_append(headers, accept_language);\n \n@@ -1961,7 +2060,26 @@ static int http_request(const char *url,\n \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n-\tret = run_one_slot(slot, &results);\n+\thttp_code = 0;\n+\tret = run_one_slot(slot, &results, &http_code);\n+\n+\t/* remove header data fields since not all slots will use them */\n+\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, NULL);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, NULL);\n+\n+\tif (ret != HTTP_OK) {\n+\t\tif (retryable_code(http_code) && retry_cnt < http_retry_limit) {\n+\t\t\tretry_cnt++;\n+\t\t\tretry_delay_sec = http_retry_after(result_headers, retry_delay_sec);\n+\t\t\twarning(Q_(\"got HTTP response %d, retrying after %d second (%d/%d)\\n\",\n+\t\t\t\t\t \"got HTTP response %d, retrying after %d seconds (%d/%d)\\n\",\n+\t\t\t\t\t retry_delay_sec),\n+\t\t\t\thttp_code, retry_delay_sec, retry_cnt, http_retry_limit);\n+\t\t\tsleep(retry_delay_sec);\n+\n+\t\t\tgoto retry;\n+\t\t}\n+\t}\n \n \tif (options && options->content_type) {\n \t\tstruct strbuf raw = STRBUF_INIT;\ndiff --git a/http.h b/http.h\nindex 5de792ef3f..faf9f1060e 100644\n--- a/http.h\n+++ b/http.h\n@@ -99,7 +99,7 @@ void finish_all_active_slots(void);\n *\n */\n int run_one_slot(struct active_request_slot *slot,\n-\t\t struct slot_results *results);\n+\t\t struct slot_results *results, int *http_code);\n \n #ifdef USE_CURL_MULTI\n void fill_active_slots(void);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 7f44fa30fe..2657c95bcb 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -805,7 +805,7 @@ static int run_slot(struct active_request_slot *slot,\n \tif (!results)\n \t\tresults = &results_buf;\n \n-\terr = run_one_slot(slot, results);\n+\terr = run_one_slot(slot, results, NULL);\n \n \tif (err != HTTP_OK && err != HTTP_REAUTH) {\n \t\tstruct strbuf msg = STRBUF_INIT;\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 72aaed5a93..3965cd265d 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -757,13 +757,17 @@ test_expect_success 'partial clone using HTTP' '\n '\n \n test_expect_success 'partial clone using HTTP with redirect' '\n-\t_NONCE=`gen_nonce` && export _NONCE &&\n+\t_NONCE=$(gen_nonce) &&\n \tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n \tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n \tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n \tpartial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\"\n '\n \n+test_expect_success 'partial clone with retry' '\n+\tpartial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/error_ntime/$(gen_nonce)/3/429/1/smart/server\" 2>err &&\n+\ttest_i18ngrep \"got HTTP response 429\" err\n+'\n \n # DO NOT add non-httpd-specific tests here, because the last part of this\n # test script is only executed when httpd is available and enabled.\n-- \n2.28.0.1011.ga647a8990f-goog\n\n"},{"id":"407485","messageId":"xmqqblh5kggd.fsf@gitster.c.googlers.com","threadId":"54409","inReplyTo":"20201013191729.2524700-1-smcallis@google.com","subject":"Re: [PATCH v2 1/3] remote-curl: add testing for intelligent retry for HTTP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T20:40:18Z","receivedAt":"2020-10-13T20:40:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sean McAllister writes:\n\n> +# generate a process unique one-up value\n> +global_counter_for_nonce=0\n> +gen_nonce () {\n> +\tglobal_counter_for_nonce=$((global_counter_for_nonce + 1)) &&\n> +\techo \"$global_counter_for_nonce\"\n> +}\n\nThis must not be called in a subprocess if the caller truly wants\nuniqueness. May want to be described in a comment.\n\n> diff --git a/t/lib-httpd/error-ntime.sh b/t/lib-httpd/error-ntime.sh\n> new file mode 100755\n> index 0000000000..64dc878746\n> --- /dev/null\n> +++ b/t/lib-httpd/error-ntime.sh\n> @@ -0,0 +1,80 @@\n> +#!/bin/sh\n> +\n> +# Script to simulate a transient error code with Retry-After header set.\n> +#\n> +# PATH_INFO must be of the form /////\n> +# (eg: /dc724af1/3/429/10/some/url)\n> +#\n> +# The value uniquely identifies the URL, since we're simulating\n> +# a stateful operation using a stateless protocol, we need a way to \"namespace\"\n> +# URLs so that they don't step on each other.\n> +#\n> +# The first times this endpoint is called, it will return the given\n> +# , and if the is non-negative, it will set the\n> +# Retry-After head to that value.\n> +#\n> +# Subsequent calls will return a 302 redirect to .\n> +#\n> +# Supported error codes are 429,502,503, and 504\n> +\n\nI thought \"error codes\" were rephrased after the first round's\nreview to some other term (which I do not recall--was it status?)?\n\n> +print_status() {\n> +\tif [ \"$1\" -eq \"302\" ]; then\n> +\t\tprintf \"Status: 302 Found\\n\"\n> +\telif [ \"$1\" -eq \"429\" ]; then\n> +\t\tprintf \"Status: 429 Too Many Requests\\n\"\n> +\telif [ \"$1\" -eq \"502\" ]; then\n> +\t\tprintf \"Status: 502 Bad Gateway\\n\"\n> +\telif [ \"$1\" -eq \"503\" ]; then\n> +\t\tprintf \"Status: 503 Service Unavailable\\n\"\n> +\telif [ \"$1\" -eq \"504\" ]; then\n> +\t\tprintf \"Status: 504 Gateway Timeout\\n\"\n> +\telse\n> +\t\tprintf \"Status: 500 Internal Server Error\\n\"\n> +\tfi\n> +\tprintf \"Content-Type: text/plain\\n\"\n> +}\n\nStyle (Documentation/CodingGuidelines).\n\n\tprint_status () {\n\t\tif test \"$1\" = \"302\"\n\t\tthen\n\t\t\tprintf \"...\";\n\t\t...\n\t}\n\nbut in this particular case, I do not see why we want if/else\ncascade. Perhaps\n\n\tprint_status () {\n\t\tcase \"$1\" in\n\t\t302)\tprintf \"Status: 302 Found\\n\" ;;\n\t\t429)\t... ;;\n\t\t...\n\t\t*)\tprintf \"Status: 500 Internal Server Error\\n\" ;;\n\t\tesac\n\t\tprintf \"Content-Type: text/plain\\n\";\n\t}\n\nwould be more standard?\n\nAlso I am not sure why we want \"printf ...\\n\" not \"echo\" here. If\nwe want to talk HTTP ourselves strictly, I would understand avoiding\n\"echo\" and doing \"printf ...\\r\\n\", though. If we fear DOS line\nendings coming out of localized \"echo\", and ensure we use LF line\nending even on Windows and Cygwin, it is sort of understandable but\nif that is what is going on, that does not explain a lone \"echo\"\nat the end of the caller.\n\nPuzzled.\n\n +oIFS=\"$IFS\"\n> +IFS='/'\n> +set -f\n> +set -- $PATH_INFO\n> +set +f\n +IFS=\"$oIFS\"\n> +\n> +# pull out first four path components\n> +shift\n> +nonce=$1\n> +times=$2\n> +code=$3\n> +retry=$4\n> +shift 4\n> +\n> +# glue the rest back together as redirect path\n> +path=\"\"\n> +while [ \"$#\" -gt \"0\" ]; do\n> +\tpath=\"${path}/$1\"\n> +\tshift\n> +done\n\nHmph. Would this work better, I wonder?\n\n\tpath=${PATH_INFO#*/}\t;# discard leading '/'\n\tnonce=${path%%/*}\tpath=${path#*/}\n\ttimes=${path%%/*}\tpath=${path#*/}\n\tcode=${path%%/*}\tpath=${path#*/}\n\tretry=${path%%/*}\tpath=${path#*/}\n\nAt least it is shorter and easier to read.\n\n> +# leave a cookie for this request/retry count\n> +state_file=\"request_${REMOTE_ADDR}_${nonce}_${times}_${code}_${retry}\"\n> +\n> +if [ ! -f \"$state_file\" ]; then\n> +\techo 0 > \"$state_file\"\n> +fi\n\nStyle (Documentation/CodingGuidelines, look for \"For shell scripts\nspecifically\").\n\n - use \"test\" not \"[]\";\n\n - don't write \";then\" on the same line (rule of thumb. you should\n be able to write your shell scripts without semicolon except for\n double-semicolons in case/esac statements)\n\n - don't leave SP between redirection operator '>' and its target\n file, i.e. write 'echo 0 >\"$state_file\"'.\n\n> +read -r cnt < \"$state_file\"\n> +if [ \"$cnt\" -lt \"$times\" ]; then\n> +\techo $((cnt+1)) > \"$state_file\"\n> +\n> +\t# return error\n> +\tprint_status \"$code\"\n> +\tif [ \"$retry\" -ge \"0\" ]; then\n> +\t\tprintf \"Retry-After: %s\\n\" \"$retry\"\n> +\tfi\n> +else\n> +\t# redirect\n> +\tprint_status 302\n> +\tprintf \"Location: %s?%s\\n\" \"$path\" \"${QUERY_STRING}\"\n> +fi\n> +\n> +echo\n\nThis \"echo\" to the client also puzzles me further, after seeing\npuzzling use of \"printf ...\\n\" earlier.\n\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 7df3c5373a..72aaed5a93 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -756,6 +756,15 @@ test_expect_success 'partial clone using HTTP' '\n> \tpartial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/smart/server\"\n> '\n> \n> +test_expect_success 'partial clone using HTTP with redirect' '\n> +\t_NONCE=`gen_nonce` && export _NONCE &&\n> +\tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n> +\tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n> +\tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n\nWhy do we need to test \"curl\" here? Is this remnant of debugging of\nthe server side?\n\n> +\tpartial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\"\n> +'\n> +\n> +\n> # DO NOT add non-httpd-specific tests here, because the last part of this\n> # test script is only executed when httpd is available and enabled.\n"},{"id":"407486","messageId":"xmqq7drtkg6o.fsf@gitster.c.googlers.com","threadId":"54409","inReplyTo":"20201013191729.2524700-2-smcallis@google.com","subject":"Re: [PATCH v2 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T20:46:07Z","receivedAt":"2020-10-13T20:46:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sean McAllister writes:\n\n> CURLOPT_FILE has been deprecated since 2003.\n\nI thought that Dscho already mention this, but updating the above\ndescription to mention that _WRITEDATA was introduce to overtake\n_FILE as an equivalent in the same timeframe would be more helpful\nto readers.\n\n\n> Signed-off-by: Sean McAllister \n> ---\n> http-push.c | 6 +++---\n> http-walker.c | 2 +-\n> http.c | 6 +++---\n> remote-curl.c | 4 ++--\n> 4 files changed, 9 insertions(+), 9 deletions(-)\n>\n> diff --git a/http-push.c b/http-push.c\n> index 6a4a43e07f..2e6fee3305 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -894,7 +894,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n> \tslot->results = &results;\n> \tcurl_setup_http(slot->curl, url, DAV_LOCK, &out_buffer, fwrite_buffer);\n> \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &in_buffer);\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);\n> ...\n"},{"id":"407487","messageId":"20201013205841.GA3678071@coredump.intra.peff.net","threadId":"54409","inReplyTo":"xmqq7drtkg6o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-13T20:58:41Z","receivedAt":"2020-10-13T20:58:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 13, 2020 at 01:46:07PM -0700, Junio C Hamano wrote:\n\n> Sean McAllister writes:\n> \n> > CURLOPT_FILE has been deprecated since 2003.\n> \n> I thought that Dscho already mention this, but updating the above\n> description to mention that _WRITEDATA was introduce to overtake\n> _FILE as an equivalent in the same timeframe would be more helpful\n> to readers.\n\nYes. But more important:\n\n - when is _FILE going away (or has it already in some versions)?\n\n - when did _WRITEDATA appear?\n\nIOW, as a reviewer I would want to make sure that we are not losing\nsupport for any reasonable version of libcurl, or that we are at least\ngetting something in return (fixing an incompatibility with newer\nversions).\n\nFrom the link Dscho dug up it looks like the answer to the second one is\n\"long enough not to care\", but the commit message should make that\nplain.\n\n-Peff\n"},{"id":"407488","messageId":"xmqq362hkew3.fsf@gitster.c.googlers.com","threadId":"54409","inReplyTo":"20201013191729.2524700-3-smcallis@google.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T21:14:04Z","receivedAt":"2020-10-13T21:14:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sean McAllister writes:\n\n> test_expect_success 'partial clone using HTTP with redirect' '\n> -\t_NONCE=`gen_nonce` && export _NONCE &&\n> +\t_NONCE=$(gen_nonce) &&\n> \tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n> \tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n> \tcurl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n> \tpartial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\"\n\nIt also bothers me somewhat that we waste 40 seconds waiting to\ncomplete this test, most of the time just sleeping.\n\nNow, do we still need gen_nonce in the test library, now there is\nonly one or two tests, both in a single test script?\n\nThanks.\n"},{"id":"407489","messageId":"20201013211453.GB3678071@coredump.intra.peff.net","threadId":"54409","inReplyTo":"20201013191729.2524700-3-smcallis@google.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-13T21:14:53Z","receivedAt":"2020-10-13T21:14:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 13, 2020 at 01:17:29PM -0600, Sean McAllister wrote:\n\n> Some HTTP response codes indicate a server state that can support\n> retrying the request rather than immediately erroring out. The server\n> can also provide information about how long to wait before retries to\n> via the Retry-After header. So check the server response and retry\n> some reasonable number of times before erroring out to better accomodate\n> transient errors.\n> \n> Exiting immediately becomes irksome when pulling large multi-repo code\n> bases such as Android or Chromium, as often the entire fetch operation\n> has to be restarted from the beginning due to an error in one repo. If\n> we can reduce how often that occurs, then it's a big win.\n\nI had hoped that libcurl might have some retry mechanisms, since the\ncurl command-line tool has several --retry-* options. But it looks like\nthat is all only at the tool level, and the library code doesn't know\nanything about it. So we are stuck driving the process ourselves.\n\nI do think you could be leveraging CURLINFO_RETRY_AFTER rather than\nimplementing your own header parsing, though.\n\n> static int http_request(const char *url,\n> \t\t\tvoid *result, int target,\n> \t\t\tconst struct http_get_options *options)\n> {\n\nIt looks like you trigger retries only from this function. But this\ndoesn't cover all http requests that Git makes. That might be sufficient\nfor your purposes (I think it would catch all of the initial contact),\nbut it might not (it probably doesn't cover subsequent POSTs for fetch\nnegotiation nor pack push; likewise I'm not sure if it covers much of\nanything after v2 stateless-connect is established).\n\n> \tstruct active_request_slot *slot;\n> \tstruct slot_results results;\n> -\tstruct curl_slist *headers = http_copy_default_headers();\n> +\tstruct curl_slist *headers;\n\nSo here we stop copying the headers at the top of the function...\n\n> [...]\n> +retry:\n> [...]\n> +\theaders = http_copy_default_headers();\n> \tif (accept_language)\n> \t\theaders = curl_slist_append(headers, accept_language);\n\nAnd instead set them up totally here. Which make some sense, because we\nwouldn't want to append accept_language over and over. But who frees the\nold ones? There is a call to curl_slist_free_all(headers) later in the\nfunction, but it's after your \"goto retry\". So I think each retry would\nleak another copy of the list.\n\nThe ideal thing would probably be to create the header list once, and\nthen use it for each retry. That would require reordering some of the\nsetup. If that's too much, then it would be OK to just create a new list\nfrom scratch on each call. Though in the latter case I suspect it may be\nsimpler to wrap the whole function, like:\n\n static int http_request(...)\n {\n\tint try;\n\tint result;\n\tfor (try = 0; try < max_retries; i++) {\n\t\tresult = http_request_try(...);\n\t\tif (...result is not retryable...)\n\t\t\tbreak;\n\t}\n\treturn result;\n }\n\nand then we'd know that the single-try function just needs to be\nself-contained, without worrying about gotos jumping around in it.\n\n-Peff\n"},{"id":"407492","messageId":"20201013234502.GB490427@camp.crustytoothpaste.net","threadId":"54409","inReplyTo":"20201013211453.GB3678071@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-10-13T23:45:02Z","receivedAt":"2020-10-13T23:45:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-10-13 at 21:14:53, Jeff King wrote:\n> On Tue, Oct 13, 2020 at 01:17:29PM -0600, Sean McAllister wrote:\n> > static int http_request(const char *url,\n> > \t\t\tvoid *result, int target,\n> > \t\t\tconst struct http_get_options *options)\n> > {\n> \n> It looks like you trigger retries only from this function. But this\n> doesn't cover all http requests that Git makes. That might be sufficient\n> for your purposes (I think it would catch all of the initial contact),\n> but it might not (it probably doesn't cover subsequent POSTs for fetch\n> negotiation nor pack push; likewise I'm not sure if it covers much of\n> anything after v2 stateless-connect is established).\n\nYeah, I was about to mention the same thing. It looks like we cover\nonly a subset of requests. Moreover, I think this feature is going to\npractically fail in some cases and we need to either document that\nclearly or abandon this effort.\n\nIn remote-curl.c, we have post_rpc, which does a POST request to upload\ndata for a push. However, if the data is larger than the buffer, we\nstream it using chunked transfer-encoding. Because we're reading from a\npipe, that data cannot be retried: the pack-objects stream will have\nended.\n\nThat's why we have code to force Expect: 100-continue for Kerberos\n(Negotiate): it can require a 401 response from the server with valid\ndata in order to send a valid Authorization header, and without the 100\nContinue response, we'd have uploaded all the data just to get the 401\nresponse, leading to a failed push.\n\nThe only possible alternative to this is to increase the buffer size\n(http.postBuffer) and I definitely don't want to encourage people to do\nthat. People already get the mistaken idea that that's a magic salve\nfor all push problems and end up needlessly allocating gigabytes of\nmemory every time they push. Encouraging people to waste memory because\nthe server might experience a problem puts the costs of unreliability on\nthe users instead of on the server operators where it belongs.\n\nSo the only sane thing to do here is to make this operation work only\nfor fetch requests, since they are the only thing that can be safely\nretried in the general case without consuming excessive resources. As a\nresult, we may want to add appropriate tests for the push case that we\ndon't retry those requests.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"407498","messageId":"alpine.DEB.2.20.2010132309250.7554@tvnag.unkk.fr","threadId":"54409","inReplyTo":"20201013205841.GA3678071@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2020-10-13T21:16:58Z","receivedAt":"2020-10-14T09:24:38Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Tue, 13 Oct 2020, Jeff King wrote:\n\nLet me just inject myself here and comment on two curl things.\n\n> - when is _FILE going away (or has it already in some versions)?\n\nIt will be kept around *at least* for as long as libcurl supports the version \n7 API: for the forseeable future. Posssibly decades.\n\n> - when did _WRITEDATA appear?\n\nAll curl symbols ever introduced are documented clearly in which version they \nappeared. See:\n\n https://curl.haxx.se/libcurl/c/symbols-in-versions.html\n\nTo map the version numbers to release dates, this second table comes handy:\n\n https://curl.haxx.se/docs/releases.html\n\nCURLOPT_WRITEDATA was added in curl 7.9.7, shipped on May 10 2002.\n\nIn the curl project we often see users use *very* old versions but 18 years is \nlonger than even the most conservative users I've seen...\n\n-- \n\n / daniel.haxx.se (with my curl hat on)\n"},{"id":"407509","messageId":"CAM4o00e1-5RqKXEVHk9u8JJQNtcKx_tNBpbyD_6B_+Wuw_bD4w@mail.gmail.com","threadId":"54409","inReplyTo":"xmqq362hkew3.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-14T14:28:12Z","receivedAt":"2020-10-14T14:28:31Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"On Tue, Oct 13, 2020 at 3:14 PM Junio C Hamano wrote:\n>\n> Sean McAllister writes:\n>\n> > test_expect_success 'partial clone using HTTP with redirect' '\n> > - _NONCE=`gen_nonce` && export _NONCE &&\n> > + _NONCE=$(gen_nonce) &&\n> > curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n> > curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n> > curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n> > partial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\"\n>\n> It also bothers me somewhat that we waste 40 seconds waiting to\n> complete this test, most of the time just sleeping.\n>\n> Now, do we still need gen_nonce in the test library, now there is\n> only one or two tests, both in a single test script?\n>\n> Thanks.\n\nIn the last patch of the series I actually knocked these down to 1\nsecond because the delay was obnoxious, I agree. I'll update it so\nthat it's done in this patch and I'll move gen_nonce.\nI'll be a little bit delayed with v3 since my workstation decided to\nexplode last night :(\n"},{"id":"407510","messageId":"CAM4o00cFdD2mcnm8SmupTSrWeetUN+pC_ZFNQijdia6uGGpYPQ@mail.gmail.com","threadId":"54409","inReplyTo":"xmqq7drtkg6o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-14T14:29:13Z","receivedAt":"2020-10-14T14:29:33Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":">\n> Sean McAllister writes:\n>\n> > CURLOPT_FILE has been deprecated since 2003.\n>\n> I thought that Dscho already mention this, but updating the above\n> description to mention that _WRITEDATA was introduce to overtake\n> _FILE as an equivalent in the same timeframe would be more helpful\n> to readers.\n>\n>\nYes he did, I'll update the commit message to give more history on the\nchange and why it's safe.\n\n> > Signed-off-by: Sean McAllister \n> > ---\n> > http-push.c | 6 +++---\n> > http-walker.c | 2 +-\n> > http.c | 6 +++---\n> > remote-curl.c | 4 ++--\n> > 4 files changed, 9 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/http-push.c b/http-push.c\n> > index 6a4a43e07f..2e6fee3305 100644\n> > --- a/http-push.c\n> > +++ b/http-push.c\n> > @@ -894,7 +894,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n> > slot->results = &results;\n> > curl_setup_http(slot->curl, url, DAV_LOCK, &out_buffer, fwrite_buffer);\n> > curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);\n> > - curl_easy_setopt(slot->curl, CURLOPT_FILE, &in_buffer);\n> > + curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);\n> > ...\n"},{"id":"407513","messageId":"20201014145709.GA12589@coredump.intra.peff.net","threadId":"54409","inReplyTo":"alpine.DEB.2.20.2010132309250.7554@tvnag.unkk.fr","subject":"Re: [PATCH v2 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-14T14:57:09Z","receivedAt":"2020-10-14T14:57:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 13, 2020 at 11:16:58PM +0200, Daniel Stenberg wrote:\n\n> Let me just inject myself here and comment on two curl things.\n> \n> > - when is _FILE going away (or has it already in some versions)?\n> \n> It will be kept around *at least* for as long as libcurl supports the\n> version 7 API: for the forseeable future. Posssibly decades.\n\nThanks. That's exactly the level of carefulness I expected from libcurl. :)\n\nSo this definitely isn't an urgent change, but given the date that\nWRITEDATA was introduced, there's very little downside to using it. So\nthe argument for the commit message is mostly that it's a readability\nimprovement.\n\n-Peff\n"},{"id":"407515","messageId":"20201014151714.GB12589@coredump.intra.peff.net","threadId":"54409","inReplyTo":"20201013234502.GB490427@camp.crustytoothpaste.net","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-14T15:17:14Z","receivedAt":"2020-10-14T15:17:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 13, 2020 at 11:45:02PM +0000, brian m. carlson wrote:\n\n> Yeah, I was about to mention the same thing. It looks like we cover\n> only a subset of requests. Moreover, I think this feature is going to\n> practically fail in some cases and we need to either document that\n> clearly or abandon this effort.\n> \n> In remote-curl.c, we have post_rpc, which does a POST request to upload\n> data for a push. However, if the data is larger than the buffer, we\n> stream it using chunked transfer-encoding. Because we're reading from a\n> pipe, that data cannot be retried: the pack-objects stream will have\n> ended.\n\nRight, this is the large_request code path there. We use the same\nfunction for fetches, too, though perhaps it's less likely that a\nnegotiation request will exceed the post buffer size.\n\nWe do send a probe rpc in this case, which lets us handle an HTTP 401\nauth request. We _could_ retry on errors to the probe rpc, but I'm not\nsure if it really accomplishes that much. The interesting thing is\nwhether the actual request with content goes through. If retrying\nmagically fixes things, there's no reason to think that the actual\nrequest is any less likely to intermittently fail than the probe request\n(in fact I'd expect it to fail more).\n\nIt would be possible to restart even these large requests. Obviously we\ncould spool the contents to disk in order to replay it. But that carries\na cost for the common case that we either succeed or definitely fail on\nthe first case, and never use the spooled copy.\n\nAnother option is to simply restart the Git process that is generating\nthe data that we're piping. But that has to happen outside of\npost_rpc(); only the caller knows the right way to invoke that again.\nAnd we kind of already have that: just run the Git command again.\nI know that sounds a little dismissive, but it has really been our\nrecommended retry strategy for ages[1].\n\nSo I'd wonder in Sean's use case why just restarting the whole Git\nprocess isn't a workable solution. It wouldn't respect Retry-After, but\nit might be reasonable to surface that header's value to the caller so\nit can act appropriately (and I guess likewise whether we saw an error\nthat implies retrying might work).\n\nAll of this is written from the perspective of v1. In v2, we do a lot\nmore blind packet-shuffling (see remote-curl.c:stateless_connect()). I\nsuspect it makes any kind of retry at the level of the http code much\nharder. Whereas just restarting the Git command would probably work just\nfine.\n\n-Peff\n\n[1] I think git-submodule will retry failed clones, for example. TBH, I\n have never once seen this retry accomplish anything, and it only\n wastes time and makes the output more confusing (since we see the\n failure twice). I have to admit I'm not thrilled to see more blind\n retrying for that reason.\n"},{"id":"407517","messageId":"xmqqmu0oj0dg.fsf@gitster.c.googlers.com","threadId":"54409","inReplyTo":"CAM4o00e1-5RqKXEVHk9u8JJQNtcKx_tNBpbyD_6B_+Wuw_bD4w@mail.gmail.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-14T15:25:15Z","receivedAt":"2020-10-14T15:25:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sean McAllister writes:\n\n> In the last patch of the series I actually knocked these down to 1\n> second because the delay was obnoxious, I agree. I'll update it so\n> that it's done in this patch and I'll move gen_nonce.\n> I'll be a little bit delayed with v3 since my workstation decided to\n> explode last night :(\n\nOuch, sorry to hear that. Be safe ;-)\n\nWe are deep in pre-release feature freeze right now anyway, so no\nrush.\n\n"},{"id":"407524","messageId":"CAM4o00es=6bksufcu7UvLfPxSS-9NndD2ALe74PRvQjBdHci5g@mail.gmail.com","threadId":"54409","inReplyTo":"xmqqblh5kggd.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/3] remote-curl: add testing for intelligent retry for HTTP","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-14T16:56:39Z","receivedAt":"2020-10-14T16:56:59Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"> Sean McAllister writes:\n>\n> > +# generate a process unique one-up value\n> > +global_counter_for_nonce=0\n> > +gen_nonce () {\n> > + global_counter_for_nonce=$((global_counter_for_nonce + 1)) &&\n> > + echo \"$global_counter_for_nonce\"\n> > +}\n>\n> This must not be called in a subprocess if the caller truly wants\n> uniqueness. May want to be described in a comment.\n>\nDone.\n\n> > diff --git a/t/lib-httpd/error-ntime.sh b/t/lib-httpd/error-ntime.sh\n> > new file mode 100755\n> > index 0000000000..64dc878746\n> > --- /dev/null\n> > +++ b/t/lib-httpd/error-ntime.sh\n> > @@ -0,0 +1,80 @@\n> > +#!/bin/sh\n> > +\n> > +# Script to simulate a transient error code with Retry-After header set.\n> > +#\n> > +# PATH_INFO must be of the form /////\n> > +# (eg: /dc724af1/3/429/10/some/url)\n> > +#\n> > +# The value uniquely identifies the URL, since we're simulating\n> > +# a stateful operation using a stateless protocol, we need a way to \"namespace\"\n> > +# URLs so that they don't step on each other.\n> > +#\n> > +# The first times this endpoint is called, it will return the given\n> > +# , and if the is non-negative, it will set the\n> > +# Retry-After head to that value.\n> > +#\n> > +# Subsequent calls will return a 302 redirect to .\n> > +#\n> > +# Supported error codes are 429,502,503, and 504\n> > +\n>\n> I thought \"error codes\" were rephrased after the first round's\n> review to some other term (which I do not recall--was it status?)?\n>\nYes you're right, fixed to reflect that and adjust formatting.\n\n> > +print_status() {\n> > + if [ \"$1\" -eq \"302\" ]; then\n> > + printf \"Status: 302 Found\\n\"\n> > + elif [ \"$1\" -eq \"429\" ]; then\n> > + printf \"Status: 429 Too Many Requests\\n\"\n> > + elif [ \"$1\" -eq \"502\" ]; then\n> > + printf \"Status: 502 Bad Gateway\\n\"\n> > + elif [ \"$1\" -eq \"503\" ]; then\n> > + printf \"Status: 503 Service Unavailable\\n\"\n> > + elif [ \"$1\" -eq \"504\" ]; then\n> > + printf \"Status: 504 Gateway Timeout\\n\"\n> > + else\n> > + printf \"Status: 500 Internal Server Error\\n\"\n> > + fi\n> > + printf \"Content-Type: text/plain\\n\"\n> > +}\n>\n> Style (Documentation/CodingGuidelines).\n>\n> print_status () {\n> if test \"$1\" = \"302\"\n> then\n> printf \"...\";\n> ...\n> }\n>\n> but in this particular case, I do not see why we want if/else\n> cascade. Perhaps\n>\n> print_status () {\n> case \"$1\" in\n> 302) printf \"Status: 302 Found\\n\" ;;\n> 429) ... ;;\n> ...\n> *) printf \"Status: 500 Internal Server Error\\n\" ;;\n> esac\n> printf \"Content-Type: text/plain\\n\";\n> }\n>\n> would be more standard?\n>\n> Also I am not sure why we want \"printf ...\\n\" not \"echo\" here. If\n> we want to talk HTTP ourselves strictly, I would understand avoiding\n> \"echo\" and doing \"printf ...\\r\\n\", though. If we fear DOS line\n> endings coming out of localized \"echo\", and ensure we use LF line\n> ending even on Windows and Cygwin, it is sort of understandable but\n> if that is what is going on, that does not explain a lone \"echo\"\n> at the end of the caller.\n>\n> Puzzled.\n>\nI modified it to use echo as the standard, it turns out apache handles\nproperly terminating lines for you\nwith CRLF. As for the lone echo, a double CRLF signals the end of the response\nheader and the start of the body. Curl doesn't behave properly without it.\n\n\n> +oIFS=\"$IFS\"\n> > +IFS='/'\n> > +set -f\n> > +set -- $PATH_INFO\n> > +set +f\n> +IFS=\"$oIFS\"\n> > +\n> > +# pull out first four path components\n> > +shift\n> > +nonce=$1\n> > +times=$2\n> > +code=$3\n> > +retry=$4\n> > +shift 4\n> > +\n> > +# glue the rest back together as redirect path\n> > +path=\"\"\n> > +while [ \"$#\" -gt \"0\" ]; do\n> > + path=\"${path}/$1\"\n> > + shift\n> > +done\n>\n> Hmph. Would this work better, I wonder?\n>\n> path=${PATH_INFO#*/} ;# discard leading '/'\n> nonce=${path%%/*} path=${path#*/}\n> times=${path%%/*} path=${path#*/}\n> code=${path%%/*} path=${path#*/}\n> retry=${path%%/*} path=${path#*/}\n>\n> At least it is shorter and easier to read.\n>\nI agree it's better. changed.\n\n\n> > +# leave a cookie for this request/retry count\n> > +state_file=\"request_${REMOTE_ADDR}_${nonce}_${times}_${code}_${retry}\"\n> > +\n> > +if [ ! -f \"$state_file\" ]; then\n> > + echo 0 > \"$state_file\"\n> > +fi\n>\n> Style (Documentation/CodingGuidelines, look for \"For shell scripts\n> specifically\").\n>\n> - use \"test\" not \"[]\";\n>\n> - don't write \";then\" on the same line (rule of thumb. you should\n> be able to write your shell scripts without semicolon except for\n> double-semicolons in case/esac statements)\n>\n> - don't leave SP between redirection operator '>' and its target\n> file, i.e. write 'echo 0 >\"$state_file\"'.\n>\nDone. I went back over the guidelines and tried to follow everything.\n\n> > +read -r cnt < \"$state_file\"\n> > +if [ \"$cnt\" -lt \"$times\" ]; then\n> > + echo $((cnt+1)) > \"$state_file\"\n> > +\n> > + # return error\n> > + print_status \"$code\"\n> > + if [ \"$retry\" -ge \"0\" ]; then\n> > + printf \"Retry-After: %s\\n\" \"$retry\"\n> > + fi\n> > +else\n> > + # redirect\n> > + print_status 302\n> > + printf \"Location: %s?%s\\n\" \"$path\" \"${QUERY_STRING}\"\n> > +fi\n> > +\n> > +echo\n>\n> This \"echo\" to the client also puzzles me further, after seeing\n> puzzling use of \"printf ...\\n\" earlier.\n>\nSee earlier comment, we need a second CRLF to end the HTTP header.\nBefore I added this, curl was very unhappy.\n\n> > diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> > index 7df3c5373a..72aaed5a93 100755\n> > --- a/t/t5601-clone.sh\n> > +++ b/t/t5601-clone.sh\n> > @@ -756,6 +756,15 @@ test_expect_success 'partial clone using HTTP' '\n> > partial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/smart/server\"\n> > '\n> >\n> > +test_expect_success 'partial clone using HTTP with redirect' '\n> > + _NONCE=`gen_nonce` && export _NONCE &&\n> > + curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n> > + curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n> > + curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" &&\n>\n> Why do we need to test \"curl\" here? Is this remnant of debugging of\n> the server side?\n>\nAt this point in the patch set the retry logic isn't implemented yet.\nSo this triggers the first error status manually\nand then clones the repo using the same URL to verify that clone still\nworks when returning a 302 redirect status.\nI've modified it in v3 so that it only has to do the manual call once though.\n\n> > + partial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\"\n> > +'\n> > +\n> > +\n> > # DO NOT add non-httpd-specific tests here, because the last part of this\n> > # test script is only executed when httpd is available and enabled.\n"},{"id":"407527","messageId":"CAM4o00eZkkqwB=8+YCA_p1oY-V7F4EhdsqqqtToXjADXo2aZ_w@mail.gmail.com","threadId":"54409","inReplyTo":"xmqq7drtkg6o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-14T17:11:08Z","receivedAt":"2020-10-14T17:11:27Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":">\n> Sean McAllister writes:\n>\n> > CURLOPT_FILE has been deprecated since 2003.\n>\n> I thought that Dscho already mention this, but updating the above\n> description to mention that _WRITEDATA was introduce to overtake\n> _FILE as an equivalent in the same timeframe would be more helpful\n> to readers.\n>\n>\nI've updated the commit message in v3 to explain more about the\nhistory and details here.\n\n> > Signed-off-by: Sean McAllister \n> > ---\n> > http-push.c | 6 +++---\n> > http-walker.c | 2 +-\n> > http.c | 6 +++---\n> > remote-curl.c | 4 ++--\n> > 4 files changed, 9 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/http-push.c b/http-push.c\n> > index 6a4a43e07f..2e6fee3305 100644\n> > --- a/http-push.c\n> > +++ b/http-push.c\n> > @@ -894,7 +894,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n> > slot->results = &results;\n> > curl_setup_http(slot->curl, url, DAV_LOCK, &out_buffer, fwrite_buffer);\n> > curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);\n> > - curl_easy_setopt(slot->curl, CURLOPT_FILE, &in_buffer);\n> > + curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);\n> > ...\n"},{"id":"407540","messageId":"CAPc5daW4oqQHNJJH-jTJNaqiNJDLdtscfvNMpgQUOJhDf-Jj1Q@mail.gmail.com","threadId":"54409","inReplyTo":"CAM4o00es=6bksufcu7UvLfPxSS-9NndD2ALe74PRvQjBdHci5g@mail.gmail.com","subject":"Re: [PATCH v2 1/3] remote-curl: add testing for intelligent retry for HTTP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-14T18:13:57Z","receivedAt":"2020-10-14T18:14:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> I modified it to use echo as the standard, it turns out apache handles\n> properly terminating lines for you\n> with CRLF. As for the lone echo, a double CRLF signals the end of the response\n> header and the start of the body. Curl doesn't behave properly without it.\n\nYou misunderstood me. I wasn't questioning the need for a blank line.\nI found the inconsistency puzzling that all the previous ones (above)\nwere done with printf and the later one was done with echo.\n"},{"id":"407541","messageId":"CAM4o00er7HiGwP+-X4t_Z-HsEZMeihLP2x3TqTWGphCdtM-arQ@mail.gmail.com","threadId":"54409","inReplyTo":"CAPc5daW4oqQHNJJH-jTJNaqiNJDLdtscfvNMpgQUOJhDf-Jj1Q@mail.gmail.com","subject":"Re: [PATCH v2 1/3] remote-curl: add testing for intelligent retry for HTTP","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-14T18:21:47Z","receivedAt":"2020-10-14T18:22:06Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"On Wed, Oct 14, 2020 at 12:14 PM Junio C Hamano wrote:\n>\n> > I modified it to use echo as the standard, it turns out apache handles\n> > properly terminating lines for you\n> > with CRLF. As for the lone echo, a double CRLF signals the end of the response\n> > header and the start of the body. Curl doesn't behave properly without it.\n>\n> You misunderstood me. I wasn't questioning the need for a blank line.\n> I found the inconsistency puzzling that all the previous ones (above)\n> were done with printf and the later one was done with echo.\n\nAh yes that was just sloppy on my part =D\n"},{"id":"407544","messageId":"CAM4o00eCKP1+f=xBw4LJfNqrstEnutAe9StxknW9qLMtPBdhKQ@mail.gmail.com","threadId":"54409","inReplyTo":"20201013211453.GB3678071@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-14T19:09:00Z","receivedAt":"2020-10-14T19:09:21Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"On Tue, Oct 13, 2020 at 3:14 PM Jeff King wrote:\n>\n> On Tue, Oct 13, 2020 at 01:17:29PM -0600, Sean McAllister wrote:\n>\n> > Exiting immediately becomes irksome when pulling large multi-repo code\n> > bases such as Android or Chromium, as often the entire fetch operation\n> > has to be restarted from the beginning due to an error in one repo. If\n> > we can reduce how often that occurs, then it's a big win.\n>\n> I had hoped that libcurl might have some retry mechanisms, since the\n> curl command-line tool has several --retry-* options. But it looks like\n> that is all only at the tool level, and the library code doesn't know\n> anything about it. So we are stuck driving the process ourselves.\n>\n> I do think you could be leveraging CURLINFO_RETRY_AFTER rather than\n> implementing your own header parsing, though.\n>\nAh I didn't know about CURLINFO_RETRY_AFTER, I'll look at that and use\nit if I can.\n\n> > static int http_request(const char *url,\n> > void *result, int target,\n> > const struct http_get_options *options)\n> > {\n>\n> It looks like you trigger retries only from this function. But this\n> doesn't cover all http requests that Git makes. That might be sufficient\n> for your purposes (I think it would catch all of the initial contact),\n> but it might not (it probably doesn't cover subsequent POSTs for fetch\n> negotiation nor pack push; likewise I'm not sure if it covers much of\n> anything after v2 stateless-connect is established).\n>\nYou're right that I only trigger from this function. I've since removed them\nin response to feedback on having too many tests, but I originally tested this\nwith:\n t5539-fetch-http-shallow.sh\n t5540-http-push-webdav.sh\n t5541-http-push-smart.sh\n t5550-http-fetch-dumb.sh\n t5551-http-fetch-smart.sh\n t5601-clone.sh\n\nI'd have to look at the packet logs to see exactly what each of those\nprotocols is doing, but it seemed to cover _most_ of what they were doing.\n\nDefinitely open to adding retries in other places though.\n\n> > struct active_request_slot *slot;\n> > struct slot_results results;\n> > - struct curl_slist *headers = http_copy_default_headers();\n> > + struct curl_slist *headers;\n>\n> So here we stop copying the headers at the top of the function...\n>\n> > [...]\n> > +retry:\n> > [...]\n> > + headers = http_copy_default_headers();\n> > if (accept_language)\n> > headers = curl_slist_append(headers, accept_language);\n>\n> And instead set them up totally here. Which make some sense, because we\n> wouldn't want to append accept_language over and over. But who frees the\n> old ones? There is a call to curl_slist_free_all(headers) later in the\n> function, but it's after your \"goto retry\". So I think each retry would\n> leak another copy of the list.\n>\n> The ideal thing would probably be to create the header list once, and\n> then use it for each retry. That would require reordering some of the\n> setup. If that's too much, then it would be OK to just create a new list\n> from scratch on each call. Though in the latter case I suspect it may be\n> simpler to wrap the whole function, like:\n>\n> static int http_request(...)\n> {\n> int try;\n> int result;\n> for (try = 0; try < max_retries; i++) {\n> result = http_request_try(...);\n> if (...result is not retryable...)\n> break;\n> }\n> return result;\n> }\n>\n> and then we'd know that the single-try function just needs to be\n> self-contained, without worrying about gotos jumping around in it.\n>\n> -Peff\nI like this idea, I've refactored it to do just this.\n"},{"id":"407545","messageId":"CAM4o00fO++JMNdES03JKRyxmZtiLjkrJ2nux0-TZYBdH2+0ijA@mail.gmail.com","threadId":"54409","inReplyTo":"CAM4o00eCKP1+f=xBw4LJfNqrstEnutAe9StxknW9qLMtPBdhKQ@mail.gmail.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-14T19:10:46Z","receivedAt":"2020-10-14T19:11:06Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"On Wed, Oct 14, 2020 at 1:09 PM Sean McAllister wrote:\n>\n> On Tue, Oct 13, 2020 at 3:14 PM Jeff King wrote:\n> >\n> > I do think you could be leveraging CURLINFO_RETRY_AFTER rather than\n> > implementing your own header parsing, though.\n> >\n> Ah I didn't know about CURLINFO_RETRY_AFTER, I'll look at that and use\n> it if I can.\n>\nI took a look, it looks like CURLINFO_RETRY_AFTER was only added in\n7.66 (September, 2019), so\nI don't think it's reasonable to rely on it for getting the\nRetry-After value in this case.\n"},{"id":"407548","messageId":"20201014193456.GA365553@coredump.intra.peff.net","threadId":"54409","inReplyTo":"CAM4o00fO++JMNdES03JKRyxmZtiLjkrJ2nux0-TZYBdH2+0ijA@mail.gmail.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-14T19:34:56Z","receivedAt":"2020-10-14T19:34:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 14, 2020 at 01:10:46PM -0600, Sean McAllister wrote:\n\n> On Wed, Oct 14, 2020 at 1:09 PM Sean McAllister wrote:\n> >\n> > On Tue, Oct 13, 2020 at 3:14 PM Jeff King wrote:\n> > >\n> > > I do think you could be leveraging CURLINFO_RETRY_AFTER rather than\n> > > implementing your own header parsing, though.\n> > >\n> > Ah I didn't know about CURLINFO_RETRY_AFTER, I'll look at that and use\n> > it if I can.\n> >\n> I took a look, it looks like CURLINFO_RETRY_AFTER was only added in\n> 7.66 (September, 2019), so\n> I don't think it's reasonable to rely on it for getting the\n> Retry-After value in this case.\n\nI agree that's pretty recent.\n\nHow important is it that we respect it? I.e., we'd have some sane retry\nbehavior if the header is missing anyway. On older curl versions, how\nbad would it be to just use that fallback behavior all the time?\n\n-Peff\n"},{"id":"407551","messageId":"20201014195544.GA365911@coredump.intra.peff.net","threadId":"54409","inReplyTo":"20201013191729.2524700-3-smcallis@google.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-14T19:55:44Z","receivedAt":"2020-10-14T19:55:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 13, 2020 at 01:17:29PM -0600, Sean McAllister wrote:\n\n> +/*\n> + * check for a retry-after header in the given headers string, if found, then\n> + * honor it, otherwise do an exponential backoff up to the max on the current\n> + * delay\n> +*/\n> +static int http_retry_after(const struct strbuf headers, int cur_delay_sec)\n> +{\n> +\tint delay_sec;\n> +\tchar *end;\n> +\tchar* value = http_header_value(headers, \"retry-after\");\n> +\n> +\tif (value) {\n> +\t\tdelay_sec = strtol(value, &end, 0);\n> +\t\tfree(value);\n> +\t\tif (*value && *end == '\\0' && delay_sec >= 0) {\n\nThis looks at the contents of the just-freed \"value\" memory block.\n\n> +\t\t\tif (delay_sec > http_max_delay_sec) {\n> +\t\t\t\tdie(Q_(\"server requested retry after %d second,\"\n> +\t\t\t\t\t \" which is longer than max allowed\\n\",\n> +\t\t\t\t\t \"server requested retry after %d seconds,\"\n> +\t\t\t\t\t \" which is longer than max allowed\\n\", delay_sec),\n> +\t\t\t\t\tdelay_sec);\n> +\t\t\t}\n> +\t\t\treturn delay_sec;\n\nI guess there's no point in being gentle here. We could shrink the retry\ntime to our maximum allowed, but the server just told us not to bother.\nBut would this die() mask the actual http error we encountered, which is\nsurely the more interesting thing for the user?\n\nI wonder if it needs to be returning a \"do not bother retrying\" value,\nwhich presumably would cause the caller to propagate the real failure in\nthe usual way.\n\n> static int http_request(const char *url,\n> \t\t\tvoid *result, int target,\n> \t\t\tconst struct http_get_options *options)\n> {\n> \tstruct active_request_slot *slot;\n> \tstruct slot_results results;\n> -\tstruct curl_slist *headers = http_copy_default_headers();\n> +\tstruct curl_slist *headers;\n> \tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct strbuf result_headers = STRBUF_INIT;\n\nThis new result_headers strbuf is filled in for every request, but I\ndon't think us ever releasing it (whether we retry or not). So I think\nit's leaking for each request.\n\nIt sounds like you're going to rework this to put the retry loop outside\nof http_request(), so it may naturally get fixed there. But I thought it\nworth mentioning.\n\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, &result_headers);\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_buffer);\n\nAfter looking at your parsing code, I wondered if there was a way to\njust get a single header out of curl. But according to the documentation\nfor CURLOPT_HEADERFUNCTION, it will pass back individual lines anyway.\nPerhaps it would be simpler to have the callback function understand\nthat we only care about getting \"Retry-After\".\n\nThe documentation says it doesn't support header folding, but that's\nprobably OK for our purposes. It's deprecated, and your custom parsing\ndoesn't handle it either. :) And most importantly, we won't misbehave\nterribly if we see it in the wild (we'll just ignore that header).\n\n-Peff\n"},{"id":"407553","messageId":"20201015000410.GB328643@google.com","threadId":"54409","inReplyTo":"CAM4o00eZjr2apH6WO-sTvuOfR-cuiSh1yhh3j=14ZFstXDz7bA@mail.gmail.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-15T00:04:10Z","receivedAt":"2020-10-15T01:20:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSean McAllister wrote:\n> On Wed, Oct 14, 2020 at 1:34 PM Jeff King wrote:\n>> On Wed, Oct 14, 2020 at 01:10:46PM -0600, Sean McAllister wrote:\n\n>>> I took a look, it looks like CURLINFO_RETRY_AFTER was only added in\n>>> 7.66 (September, 2019), so\n>>> I don't think it's reasonable to rely on it for getting the\n>>> Retry-After value in this case.\n>>\n>> I agree that's pretty recent.\n>>\n>> How important is it that we respect it? I.e., we'd have some sane retry\n>> behavior if the header is missing anyway. On older curl versions, how\n>> bad would it be to just use that fallback behavior all the time?\n>\n> Some large projects (Android, Chrome), use git with a distributed\n> backend to feed changes to automated builders and such. We can\n> actually get into a case where DDOS mitigation kicks in and 429s start\n> going out. In that case I think it's pretty important that we honor\n> the Retry-After field so we're good citizens and whoever's running the\n> backend service has some options for traffic shaping to manage load.\n> In general you're right it doesn't matter _that_ much but in at least\n> the specific case I have in my head, it does.\n\nI see. With Peff's proposal, the backend service could still set\nRetry-After, and *modern* machines with new enough libcurl would still\nrespect it, but older machines[*] would have to use some fallback\nbehavior.\n\nI suppose that fallback behavior could be to not retry at all. That\nsounds appealing to me since it would be more maintainable (no custom\nheader parsing) and the fallback would be decreasingly relevant over\ntime as users upgrade to modern versions of libcurl and git. What do\nyou think?\n\nThanks,\nJonathan\n\n[*] This represents two cases: if CURLINFO_RETRY_AFTER is not defined,\nmeaning that libcurl-dev was too old at *build time*, and if getinfo\nreturned CURLE_UNKNOWN_OPTION, meaning that libcurl was too old at run\ntime.\n\nYou might wonder how we handle the too-old-at-build-time case. The\nusual trick is to provide a helper function with a stub\nimplementation:\n\n #ifndef CURLINFO_RETRY_AFTER\n static inline CURLcode retry_after(CURL *handle, curl_off_t *retry)\n {\n \treturn CURLE_UNKNOWN_OPTION;\n }\n #else\n static inline CURLcode retry_after(CURL *handle, curl_off_t *retry)\n {\n \treturn curl_easy_getinfo(handle, CURLINFO_RETRY_AFTER, retry);\n }\n #endif\n"},{"id":"407577","messageId":"CAM4o00eZjr2apH6WO-sTvuOfR-cuiSh1yhh3j=14ZFstXDz7bA@mail.gmail.com","threadId":"54409","inReplyTo":"20201014193456.GA365553@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-14T22:38:36Z","receivedAt":"2020-10-15T02:14:49Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"Some large projects (Android, Chrome), use git with a distributed\nbackend to feed changes to automated builders and such. We can\nactually get into a case where DDOS mitigation kicks in and 429s start\ngoing out. In that case I think it's pretty important that we honor\nthe Retry-After field so we're good citizens and whoever's running the\nbackend service has some options for traffic shaping to manage load.\nIn general you're right it doesn't matter _that_ much but in at least\nthe specific case I have in my head, it does.\n\nOn Wed, Oct 14, 2020 at 1:34 PM Jeff King wrote:\n>\n> On Wed, Oct 14, 2020 at 01:10:46PM -0600, Sean McAllister wrote:\n>\n> > On Wed, Oct 14, 2020 at 1:09 PM Sean McAllister wrote:\n> > >\n> > > On Tue, Oct 13, 2020 at 3:14 PM Jeff King wrote:\n> > > >\n> > > > I do think you could be leveraging CURLINFO_RETRY_AFTER rather than\n> > > > implementing your own header parsing, though.\n> > > >\n> > > Ah I didn't know about CURLINFO_RETRY_AFTER, I'll look at that and use\n> > > it if I can.\n> > >\n> > I took a look, it looks like CURLINFO_RETRY_AFTER was only added in\n> > 7.66 (September, 2019), so\n> > I don't think it's reasonable to rely on it for getting the\n> > Retry-After value in this case.\n>\n> I agree that's pretty recent.\n>\n> How important is it that we respect it? I.e., we'd have some sane retry\n> behavior if the header is missing anyway. On older curl versions, how\n> bad would it be to just use that fallback behavior all the time?\n>\n> -Peff\n"},{"id":"407578","messageId":"CAM4o00eefXK2CJ_FxwwVPpBKL01JsJANf+SdjCtw_0NVV82L+Q@mail.gmail.com","threadId":"54409","inReplyTo":"20201014195544.GA365911@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-14T22:46:57Z","receivedAt":"2020-10-15T02:24:46Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"On Wed, Oct 14, 2020 at 1:55 PM Jeff King wrote:\n>\n> On Tue, Oct 13, 2020 at 01:17:29PM -0600, Sean McAllister wrote:\n>\n> > +/*\n> > + * check for a retry-after header in the given headers string, if found, then\n> > + * honor it, otherwise do an exponential backoff up to the max on the current\n> > + * delay\n> > +*/\n> > +static int http_retry_after(const struct strbuf headers, int cur_delay_sec)\n> > +{\n> > + int delay_sec;\n> > + char *end;\n> > + char* value = http_header_value(headers, \"retry-after\");\n> > +\n> > + if (value) {\n> > + delay_sec = strtol(value, &end, 0);\n> > + free(value);\n> > + if (*value && *end == '\\0' && delay_sec >= 0) {\n>\n> This looks at the contents of the just-freed \"value\" memory block.\n>\nSure does, fixed in v3, disappointed that electric fence didn't catch\nthis for me...\n\n> > + if (delay_sec > http_max_delay_sec) {\n> > + die(Q_(\"server requested retry after %d second,\"\n> > + \" which is longer than max allowed\\n\",\n> > + \"server requested retry after %d seconds,\"\n> > + \" which is longer than max allowed\\n\", delay_sec),\n> > + delay_sec);\n> > + }\n> > + return delay_sec;\n>\n> I guess there's no point in being gentle here. We could shrink the retry\n> time to our maximum allowed, but the server just told us not to bother.\n> But would this die() mask the actual http error we encountered, which is\n> surely the more interesting thing for the user?\n>\n> I wonder if it needs to be returning a \"do not bother retrying\" value,\n> which presumably would cause the caller to propagate the real failure in\n> the usual way.\n>\nI've moved this check up a couple levels in v3, so that if we get too large\na retry value, then we'll print this message as a warning and quit retrying,\nwhich will unmask the underlying HTTP error.\n\n> > static int http_request(const char *url,\n> > void *result, int target,\n> > const struct http_get_options *options)\n> > {\n> > struct active_request_slot *slot;\n> > struct slot_results results;\n> > - struct curl_slist *headers = http_copy_default_headers();\n> > + struct curl_slist *headers;\n> > struct strbuf buf = STRBUF_INIT;\n> > + struct strbuf result_headers = STRBUF_INIT;\n>\n> This new result_headers strbuf is filled in for every request, but I\n> don't think us ever releasing it (whether we retry or not). So I think\n> it's leaking for each request.\n>\n> It sounds like you're going to rework this to put the retry loop outside\n> of http_request(), so it may naturally get fixed there. But I thought it\n> worth mentioning.\n>\nYes good catch, the new http_request_try version fixes it as a matter of course.\n\n> > + curl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, &result_headers);\n> > + curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_buffer);\n>\n> After looking at your parsing code, I wondered if there was a way to\n> just get a single header out of curl. But according to the documentation\n> for CURLOPT_HEADERFUNCTION, it will pass back individual lines anyway.\n> Perhaps it would be simpler to have the callback function understand\n> that we only care about getting \"Retry-After\".\n>\n> The documentation says it doesn't support header folding, but that's\n> probably OK for our purposes. It's deprecated, and your custom parsing\n> doesn't handle it either. :) And most importantly, we won't misbehave\n> terribly if we see it in the wild (we'll just ignore that header).\n>\nI'll put this in my todo pile to think on a little, it'd be nice not\nto have expand\nthe strbuf with every request, but also not a huge overhead.\n\n> -Peff\n"},{"id":"407618","messageId":"20201015161447.GB1104947@coredump.intra.peff.net","threadId":"54409","inReplyTo":"20201015000410.GB328643@google.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T16:14:47Z","receivedAt":"2020-10-15T16:14:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 14, 2020 at 05:04:10PM -0700, Jonathan Nieder wrote:\n\n> > Some large projects (Android, Chrome), use git with a distributed\n> > backend to feed changes to automated builders and such. We can\n> > actually get into a case where DDOS mitigation kicks in and 429s start\n> > going out. In that case I think it's pretty important that we honor\n> > the Retry-After field so we're good citizens and whoever's running the\n> > backend service has some options for traffic shaping to manage load.\n> > In general you're right it doesn't matter _that_ much but in at least\n> > the specific case I have in my head, it does.\n> \n> I see. With Peff's proposal, the backend service could still set\n> Retry-After, and *modern* machines with new enough libcurl would still\n> respect it, but older machines[*] would have to use some fallback\n> behavior.\n> \n> I suppose that fallback behavior could be to not retry at all. That\n> sounds appealing to me since it would be more maintainable (no custom\n> header parsing) and the fallback would be decreasingly relevant over\n> time as users upgrade to modern versions of libcurl and git. What do\n> you think?\n\nYeah, the good-citizen behavior would be to err on the side of not\nretrying. And actually, I kind of like that better anyway. The\nretryable_code() list has things like 502 in it, which aren't\nnecessarily temporary errors. If the server didn't give us a hint of\nwhen to retry, perhaps it's not a good idea to do so.\n\nThat's slightly orthogonal to the CURLINFO_RETRY_AFTER question. It\nwould mean an older Git would not retry in this case. But if you're\nprimarily interested in fixing automated builders overloading your\nsystem, it's probably not that big a deal to make sure they're up to\ndate (after all, they need the new Git, too ;) ).\n\nIf you're hoping to help random developers on various platforms, then\nmaking the feature work with older curl is more compelling. Many people\nmight upgrade their Git version there, but be stuck with an older system\nlibcurl.\n\n-Peff\n"},{"id":"407622","messageId":"20201015162343.GC1104947@coredump.intra.peff.net","threadId":"54409","inReplyTo":"CAM4o00eefXK2CJ_FxwwVPpBKL01JsJANf+SdjCtw_0NVV82L+Q@mail.gmail.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T16:23:43Z","receivedAt":"2020-10-15T16:23:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 14, 2020 at 04:46:57PM -0600, Sean McAllister wrote:\n\n> > I wonder if it needs to be returning a \"do not bother retrying\" value,\n> > which presumably would cause the caller to propagate the real failure in\n> > the usual way.\n> >\n> I've moved this check up a couple levels in v3, so that if we get too large\n> a retry value, then we'll print this message as a warning and quit retrying,\n> which will unmask the underlying HTTP error.\n\nThanks, that sounds much better.\n\n> > After looking at your parsing code, I wondered if there was a way to\n> > just get a single header out of curl. But according to the documentation\n> > for CURLOPT_HEADERFUNCTION, it will pass back individual lines anyway.\n> > Perhaps it would be simpler to have the callback function understand\n> > that we only care about getting \"Retry-After\".\n> >\n> > The documentation says it doesn't support header folding, but that's\n> > probably OK for our purposes. It's deprecated, and your custom parsing\n> > doesn't handle it either. :) And most importantly, we won't misbehave\n> > terribly if we see it in the wild (we'll just ignore that header).\n> >\n> I'll put this in my todo pile to think on a little, it'd be nice not\n> to have expand the strbuf with every request, but also not a huge\n> overhead.\n\nI was less concerned with the overhead of the strbuf (http requests are\npretty heavyweight already) and more that it could simplify your parsing\nif you could just do it left-to-right on a single line:\n\n\tchar *line = xmemdupz(buffer, size);\n\tconst char *p = line;\n\tif (skip_iprefix(p, \"retry-after:\", &p)) {\n\t\tchar *end;\n\t\twhile (isspace(*p))\n\t\t\tp++;\n\t\topts->retry_after = strtol(p, &end, 10);\n\t\t/* if you want to be pedantic */\n\t\tif (*end && *end != '\\r' && *end != '\\n')\n\t\t\topts->retry_after = 0; /* warn, too? */\n\t}\n\nIf you want to be clever, you could probably avoid the extra allocation,\nbut I think being able to parse with simple string functions makes it\nmuch more obvious that we don't walk off the end of the input.\n\n-Peff\n"},{"id":"407623","messageId":"xmqqsgafh2y2.fsf@gitster.c.googlers.com","threadId":"54409","inReplyTo":"20201015000410.GB328643@google.com","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T16:24:53Z","receivedAt":"2020-10-15T16:24:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> I see. With Peff's proposal, the backend service could still set\n> Retry-After, and *modern* machines with new enough libcurl would still\n> respect it, but older machines[*] would have to use some fallback\n> behavior.\n>\n> I suppose that fallback behavior could be to not retry at all.\n\nYup, that is a quite attractive approach. We'll let the passage of\ntime work for us.\n"},{"id":"407626","messageId":"CAM4o00ehgWp6nHi9yN7TyvoNu593OaMWW1f9FA1q9Cw+HdLZeg@mail.gmail.com","threadId":"54409","inReplyTo":"20201015162343.GC1104947@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] http: automatically retry some requests","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-15T16:33:52Z","receivedAt":"2020-10-15T16:34:10Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":">\n> I was less concerned with the overhead of the strbuf (http requests are\n> pretty heavyweight already) and more that it could simplify your parsing\n> if you could just do it left-to-right on a single line:\n>\n> char *line = xmemdupz(buffer, size);\n> const char *p = line;\n> if (skip_iprefix(p, \"retry-after:\", &p)) {\n> char *end;\n> while (isspace(*p))\n> p++;\n> opts->retry_after = strtol(p, &end, 10);\n> /* if you want to be pedantic */\n> if (*end && *end != '\\r' && *end != '\\n')\n> opts->retry_after = 0; /* warn, too? */\n> }\n>\n> If you want to be clever, you could probably avoid the extra allocation,\n> but I think being able to parse with simple string functions makes it\n> much more obvious that we don't walk off the end of the input.\n>\nThe latest code effectively does this per Dosch' suggestion. No more\nstring splitting, it just\nparses it as a single big string as you mention. It sounds like we're\nquickly settling on using\nCURLINFO_RETRY_AFTER when available, and not retrying otherwise, so it\nmight be OBE.\n"}]}