{"thread":{"id":"54400","subject":"[PATCH 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA","startedAt":"2020-10-12T18:48:19Z","lastAt":"2020-10-13T17:45:09Z","messageCount":13,"participants":["Sean McAllister","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"407363","messageId":"20201012184806.166251-2-smcallis@google.com","threadId":"54400","inReplyTo":"20201012184806.166251-1-smcallis@google.com","subject":"[PATCH 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-12T18:48:05Z","receivedAt":"2020-10-12T18:48:19Z","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":"407364","messageId":"20201012184806.166251-3-smcallis@google.com","threadId":"54400","inReplyTo":"20201012184806.166251-1-smcallis@google.com","subject":"[PATCH 3/3] http: automatically retry some requests","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-12T18:48:06Z","receivedAt":"2020-10-12T18:48:22Z","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 | 121 +++++++++++++++++++++++++++++++++-\n http.h | 13 +++-\n remote-curl.c | 2 +-\n t/t5539-fetch-http-shallow.sh | 41 ++++++++----\n t/t5540-http-push-webdav.sh | 26 +++++++-\n t/t5541-http-push-smart.sh | 15 +++++\n t/t5550-http-fetch-dumb.sh | 12 ++++\n t/t5551-http-fetch-smart.sh | 14 ++++\n t/t5601-clone.sh | 6 +-\n 10 files changed, 237 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/config/http.txt b/Documentation/config/http.txt\nindex 3968fbb697..0beec3d370 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 error 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..e41d7c5575 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 = 2;\n+static int http_max_delay = 60;\n \n static struct {\n \tconst char *name;\n@@ -219,6 +222,51 @@ 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+size_t http_header_value(\n+\tconst struct strbuf headers, const char *header, char **value)\n+{\n+\tsize_t len = 0;\n+\tstruct strbuf **lines, **line;\n+\tchar *colon = NULL;\n+\n+\tlines = strbuf_split(&headers, '\\n');\n+\tfor (line = lines; *line; line++) {\n+\t\tstrbuf_trim(*line);\n+\n+\t\t/* find colon and null it out to 'split' string */\n+\t\tcolon = strchr((*line)->buf, ':');\n+\t\tif (colon) {\n+\t\t\t*colon = '\\0';\n+\n+\t\t\tif (!strcasecmp(header, (*line)->buf)) {\n+\t\t\t\t/* move past colon and skip whitespace */\n+\t\t\t\tcolon++;\n+\t\t\t\twhile (*colon && isspace(*colon)) colon++;\n+\t\t\t\t*value = xstrdup(colon);\n+\t\t\t\tlen = strlen(*value);\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+done:\n+\tstrbuf_list_free(lines);\n+\treturn len;\n+}\n+\n static void closedown_active_slot(struct active_request_slot *slot)\n {\n \tactive_requests--;\n@@ -452,6 +500,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 +1721,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 +1731,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 +1958,55 @@ 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+/* 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 delay\n+*/\n+static int http_retry_after(const struct strbuf headers, int cur_delay) {\n+\tint len, delay;\n+\tchar *end;\n+\tchar *value;\n+\n+\tlen = http_header_value(headers, \"retry-after\", &value);\n+\tif (len) {\n+\t\tdelay = strtol(value, &end, 0);\n+\t\tif (*value && *end == 0 && delay >= 0) {\n+\t\t\tif (delay > http_max_delay) {\n+\t\t\t\tdie(Q_(\n+\t\t\t\t\t\t\"server requested retry after %d second, which is longer than max allowed\\n\",\n+\t\t\t\t\t\t\"server requested retry after %d seconds, which is longer than max allowed\\n\", delay), delay);\n+\t\t\t}\n+\t\t\tfree(value);\n+\t\t\treturn delay;\n+\t\t}\n+\t\tfree(value);\n+\t}\n+\n+\tcur_delay *= 2;\n+\treturn cur_delay >= http_max_delay ? http_max_delay : cur_delay;\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 = http_default_delay;\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 +2026,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 +2052,31 @@ 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+\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 = http_retry_after(result_headers, retry_delay);\n+\t\t\tfprintf(stderr,\n+\t\t\t Q_(\"got HTTP response %d, retrying after %d second (%d/%d)\\n\",\n+\t\t\t\t \"got HTTP response %d, retrying after %d seconds (%d/%d)\\n\",\n+\t\t\t\t\tretry_delay),\n+\t\t\t\thttp_code, retry_delay, retry_cnt, http_retry_limit);\n+\t\t\tsleep(retry_delay);\n+\n+\t\t\t// remove header data fields since not all slots will use them\n+\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, NULL);\n+\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, NULL);\n+\n+\t\t\tgoto retry;\n+\t\t}\n+\t}\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 (options && options->content_type) {\n \t\tstruct strbuf raw = STRBUF_INIT;\ndiff --git a/http.h b/http.h\nindex 5de792ef3f..60ce03801f 100644\n--- a/http.h\n+++ b/http.h\n@@ -86,6 +86,17 @@ size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);\n #endif\n \n+/*\n+ * Query the value of an HTTP header.\n+ *\n+ * If the header is found, then a newly allocate string is returned through\n+ * the value parameter, and the length is returned.\n+ *\n+ * If not found, returns 0\n+ */\n+size_t http_header_value(\n+\tconst struct strbuf headers, const char *header, char **value);\n+\n /* Slot lifecycle functions */\n struct active_request_slot *get_active_slot(void);\n int start_active_slot(struct active_request_slot *slot);\n@@ -99,7 +110,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/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh\nindex 82aa99ae87..e09083e2b3 100755\n--- a/t/t5539-fetch-http-shallow.sh\n+++ b/t/t5539-fetch-http-shallow.sh\n@@ -30,20 +30,39 @@ test_expect_success 'clone http repository' '\n \tgit clone --bare --no-local shallow \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n \tgit clone $HTTPD_URL/smart/repo.git clone &&\n \t(\n-\tcd clone &&\n-\tgit fsck &&\n-\tgit log --format=%s origin/master >actual &&\n-\tcat <expect &&\n-7\n-6\n-5\n-4\n-3\n-EOF\n-\ttest_cmp expect actual\n+\t\tcd clone &&\n+\t\tgit fsck &&\n+\t\tgit log --format=%s origin/master >actual &&\n+\t\tcat <<-\\EOF >expect &&\n+\t\t7\n+\t\t6\n+\t\t5\n+\t\t4\n+\t\t3\n+\t\tEOF\n+\t\ttest_cmp expect actual\n \t)\n '\n \n+test_expect_success 'clone http repository with flaky http' '\n+ rm -rf clone &&\n+\tgit clone $HTTPD_URL/error_ntime/`gen_nonce`/3/429/1/smart/repo.git clone 2>err &&\n+\t(\n+\t\tcd clone &&\n+\t\tgit fsck &&\n+\t\tgit log --format=%s origin/master >actual &&\n+\t\tcat <<-\\EOF >expect &&\n+\t\t7\n+\t\t6\n+\t\t5\n+\t\t4\n+\t\t3\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t) &&\n+ test_i18ngrep \"got HTTP response 429\" err\n+'\n+\n # This test is tricky. We need large enough \"have\"s that fetch-pack\n # will put pkt-flush in between. Then we need a \"have\" the server\n # does not have, it'll send \"ACK %s ready\"\ndiff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh\nindex 450321fddb..d8234d555c 100755\n--- a/t/t5540-http-push-webdav.sh\n+++ b/t/t5540-http-push-webdav.sh\n@@ -68,12 +68,36 @@ test_expect_success 'push already up-to-date' '\n \tgit push\n '\n \n+test_expect_success 'push to remote repository with packed refs and flakey server' '\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n+\t rm -rf packed-refs &&\n+\t git update-ref refs/heads/master $ORIG_HEAD &&\n+\t git --bare update-server-info) &&\n+ git remote set-url origin $HTTPD_URL/error_ntime/`gen_nonce`/3/502/1/dumb/test_repo.git &&\n+\tgit push &&\n+ git remote set-url origin $HTTPD_URL/dumb/test_repo.git &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n+\t test $HEAD = $(git rev-parse --verify HEAD))\n+'\n+\n test_expect_success 'push to remote repository with unpacked refs' '\n \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n-\t rm packed-refs &&\n+\t rm -rf packed-refs &&\n+\t git update-ref refs/heads/master $ORIG_HEAD &&\n+\t git --bare update-server-info) &&\n+\tgit push &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n+\t test $HEAD = $(git rev-parse --verify HEAD))\n+'\n+\n+test_expect_success 'push to remote repository with unpacked refs and flakey server' '\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n+\t rm -rf packed-refs &&\n \t git update-ref refs/heads/master $ORIG_HEAD &&\n \t git --bare update-server-info) &&\n+ git remote set-url origin $HTTPD_URL/error_ntime/`gen_nonce`/3/502/1/dumb/test_repo.git &&\n \tgit push &&\n+ git remote set-url origin $HTTPD_URL/dumb/test_repo.git &&\n \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n \t test $HEAD = $(git rev-parse --verify HEAD))\n '\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 187454f5dd..9b2fc62e11 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -77,6 +77,21 @@ test_expect_success 'push to remote repository (standard)' '\n \t test $HEAD = $(git rev-parse --verify HEAD))\n '\n \n+test_expect_success 'push to remote repository (flakey server)' '\n+\tcd \"$ROOT_PATH\"/test_repo_clone &&\n+\t: >path5 &&\n+\tgit add path5 &&\n+\ttest_tick &&\n+\tgit commit -m path5 &&\n+\tHEAD=$(git rev-parse --verify HEAD) &&\n+\tgit remote set-url origin $HTTPD_URL/error_ntime/`gen_nonce`/3/502/1/smart/test_repo.git &&\n+\tGIT_TRACE_CURL=true git push -v -v 2>err &&\n+\tgit remote set-url origin $HTTPD_URL/smart/test_repo.git &&\n+\tgrep \"POST git-receive-pack ([0-9]* bytes)\" err &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n+\ttest $HEAD = $(git rev-parse --verify HEAD))\n+'\n+\n test_expect_success 'push already up-to-date' '\n \tgit push\n '\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 483578b2d7..350c47097b 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -176,6 +176,18 @@ test_expect_success 'fetch changes via http' '\n \ttest_cmp file clone/file\n '\n \n+test_expect_success 'fetch changes via flakey http' '\n+\techo content >>file &&\n+\tgit commit -a -m three &&\n+\tgit push public &&\n+\t(cd clone &&\n+\t\tgit remote set-url origin $HTTPD_URL/error_ntime/`gen_nonce`/3/502/1/dumb/repo.git &&\n+\t\tgit pull 2>../err &&\n+\t\tgit remote set-url origin $HTTPD_URL/dumb/repo.git) &&\n+ test_i18ngrep \"got HTTP response 502\" err &&\n+\ttest_cmp file clone/file\n+'\n+\n test_expect_success 'fetch changes via manual http-fetch' '\n \tcp -R clone-tmpl clone2 &&\n \ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex e40e9ed52f..85d2a0e8b8 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -45,6 +45,7 @@ test_expect_success 'clone http repository' '\n \tEOF\n \tGIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\tgit clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n+ cd clone && git config pull.rebase false && cd .. &&\n \ttest_cmp file clone/file &&\n \ttr '\\''\\015'\\'' Q >file &&\n+\tgit commit -a -m two &&\n+\tgit push public &&\n+\t(cd clone &&\n+\t\tgit remote set-url origin $HTTPD_URL/error_ntime/`gen_nonce`/3/502/1/smart/repo.git &&\n+\t\tgit pull 2>../err &&\n+\t\tgit remote set-url origin $HTTPD_URL/smart/repo.git) &&\n+\ttest_cmp file clone/file &&\n+ test_i18ngrep \"got HTTP response 502\" err\n+'\n+\n+\n test_expect_success 'used upload-pack service' '\n \tcat >exp <<-\\EOF &&\n \tGET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 71d4307001..9988e3ff14 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- _NONCE=`gen_nonce` && export _NONCE &&\n+ _NONCE=`gen_nonce` &&\n curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\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+ test_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":"407369","messageId":"nycvar.QRO.7.76.6.2010122119580.50@tvgsbejvaqbjf.bet","threadId":"54400","inReplyTo":"20201012184806.166251-2-smcallis@google.com","subject":"Re: [PATCH 2/3] replace CURLOPT_FILE With CURLOPT_WRITEDATA","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-12T19:26:03Z","receivedAt":"2020-10-12T19:26:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Sean,\n\nOn Mon, 12 Oct 2020, Sean McAllister wrote:\n\n> CURLOPT_FILE has been deprecated since 2003.\n\nTo be precise, it has been aliased to `CURLOPT_WRITEDATA` in cURL v7.9.7:\nhttps://github.com/curl/curl/commit/980a47b42b95d7b9ff3378dc7b0f2e1c453fb649\n\nThe `CURLOPT_WRITEDATA` symbol became the preferred one only in v7.37.1 in\n2014, though:\nhttps://github.com/curl/curl/commit/5fcef972b289bdc7f3dbd7a55a5ada0460b74b2d\n\nCiao,\nDscho\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> \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);\n> diff --git a/http-walker.c b/http-walker.c\n> index 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>\n> diff --git a/http.c b/http.c\n> index 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);\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 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> --\n> 2.28.0.1011.ga647a8990f-goog\n>\n>\n"},{"id":"407383","messageId":"nycvar.QRO.7.76.6.2010122126280.50@tvgsbejvaqbjf.bet","threadId":"54400","inReplyTo":"20201012184806.166251-3-smcallis@google.com","subject":"Re: [PATCH 3/3] http: automatically retry some requests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-12T20:15:38Z","receivedAt":"2020-10-12T20:15:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Sean,\n\nOn Mon, 12 Oct 2020, 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\nMakes a lot of sense to me.\n\n>\n> Signed-off-by: Sean McAllister \n> ---\n> Documentation/config/http.txt | 5 ++\n> http.c | 121 +++++++++++++++++++++++++++++++++-\n> http.h | 13 +++-\n> remote-curl.c | 2 +-\n> t/t5539-fetch-http-shallow.sh | 41 ++++++++----\n> t/t5540-http-push-webdav.sh | 26 +++++++-\n> t/t5541-http-push-smart.sh | 15 +++++\n> t/t5550-http-fetch-dumb.sh | 12 ++++\n> t/t5551-http-fetch-smart.sh | 14 ++++\n> t/t5601-clone.sh | 6 +-\n> 10 files changed, 237 insertions(+), 18 deletions(-)\n>\n> diff --git a/Documentation/config/http.txt b/Documentation/config/http.txt\n> index 3968fbb697..0beec3d370 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 error codes (eg: 429,503) can reasonably be retried if\n\nPlease have a space after the comma so that it is not being mistaken for a\n6-digit number. Also, aren't they called \"status codes\"? Not all of them\nindicate errors, after all.\n\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\n> diff --git a/http.c b/http.c\n> index b3c1669388..e41d7c5575 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 = 2;\n\nShould there be a config option for that? Also, it took me some time to\nfind the code using this variable in order to find out what unit to use:\nit is seconds (not microseconds, as I had expected). Maybe this can be\ndocumented in the variable name, or at least in a comment?\n\n> +static int http_max_delay = 60;\n>\n> static struct {\n> \tconst char *name;\n> @@ -219,6 +222,51 @@ 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> +size_t http_header_value(\n\nIn the part of Git's code with which I am familiar, we avoid trying to\nbreak the line after an opening parenthesis, instead preferring to break\nafter a comma.\n\nAlso, shouldn't we make the return type `ssize_t` to allow for a negative\nvalue to indicate an error/missing header?\n\n> +\tconst struct strbuf headers, const char *header, char **value)\n> +{\n> +\tsize_t len = 0;\n> +\tstruct strbuf **lines, **line;\n> +\tchar *colon = NULL;\n> +\n> +\tlines = strbuf_split(&headers, '\\n');\n> +\tfor (line = lines; *line; line++) {\n> +\t\tstrbuf_trim(*line);\n> +\n> +\t\t/* find colon and null it out to 'split' string */\n> +\t\tcolon = strchr((*line)->buf, ':');\n> +\t\tif (colon) {\n> +\t\t\t*colon = '\\0';\n> +\n> +\t\t\tif (!strcasecmp(header, (*line)->buf)) {\n\nIf all we want is to find the given header, splitting lines seems to be a\nbit wasteful to me. We could instead search for the header directly:\n\n\tconst char *p = strcasestr(headers.buf, header), *eol;\n\tsize_t header_len = strlen(header);\n\n\twhile (p) {\n\t\tif ((p != headers.buf && p[-1] != '\\n') ||\n\t\t p[header_len] != ':') {\n\t\t\tp = strcasestr(p + header_len, header);\n\t\t\tcontinue;\n\t\t}\n\n\t\tp += header_len + 1;\n\t\twhile (isspace(*p) && *p != '\\n')\n\t\t\tp++;\n\t\teol = strchrnul(p, '\\n');\n\t\tlen = eol - p;\n\t\t*value = xstrndup(p, len);\n\t\treturn len;\n\t}\n\n> +\t\t\t\t/* move past colon and skip whitespace */\n> +\t\t\t\tcolon++;\n> +\t\t\t\twhile (*colon && isspace(*colon)) colon++;\n> +\t\t\t\t*value = xstrdup(colon);\n> +\t\t\t\tlen = strlen(*value);\n> +\t\t\t\tgoto done;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +done:\n> +\tstrbuf_list_free(lines);\n> +\treturn len;\n> +}\n> +\n> static void closedown_active_slot(struct active_request_slot *slot)\n> {\n> \tactive_requests--;\n> @@ -452,6 +500,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 +1721,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 +1731,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 +1958,55 @@ 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> +/* 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 delay\n> +*/\n\nMulti-line comments should be of this form:\n\n\t/*\n\t * Check for a retry-after header in the given headers string, if\n\t * found, then honor it, otherwise do an exponential backoff up to\n\t * the maximum on the current delay.\n\t */\n\n> +static int http_retry_after(const struct strbuf headers, int cur_delay) {\n\nFor functions, the initial opening curly bracket goes on its own line.\n\n> +\tint len, delay;\n> +\tchar *end;\n> +\tchar *value;\n\nWhy not declare `char *end, *value;`, just like `len` and `delay` are\ndeclared on the same line?\n\n> +\n> +\tlen = http_header_value(headers, \"retry-after\", &value);\n> +\tif (len) {\n> +\t\tdelay = strtol(value, &end, 0);\n> +\t\tif (*value && *end == 0 && delay >= 0) {\n\nBetter: `*end == '\\0'`\n\nAnd why `*value` here? We already called `strtol()` on it.\n\n> +\t\t\tif (delay > http_max_delay) {\n> +\t\t\t\tdie(Q_(\n\nLet's not end the line in an opening parenthesis. Instead, use C's string\ncontinuation like so:\n\n\t\t\t\tdie(Q_(\"server requested retry after %d second,\"\n\t\t\t\t \" which is longer than max allowed\\n\",\n\t\t\t\t \"server requested retry after %d \"\n\t\t\t\t \"seconds, which is longer than max \"\n\t\t\t\t \"allowed\\n\", delay), delay);\n\n> +\t\t\t\t\t\t\"server requested retry after %d second, which is longer than max allowed\\n\",\n> +\t\t\t\t\t\t\"server requested retry after %d seconds, which is longer than max allowed\\n\", delay), delay);\n> +\t\t\t}\n> +\t\t\tfree(value);\n\n`value` is not actually used after that `strtol()` call above, so let's\nrelease it right then and there.\n\n> +\t\t\treturn delay;\n> +\t\t}\n> +\t\tfree(value);\n> +\t}\n\nIf the header was found, but for some reason had an empty value, we're\nleaking `value` here.\n\n> +\n> +\tcur_delay *= 2;\n> +\treturn cur_delay >= http_max_delay ? http_max_delay : cur_delay;\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 = http_default_delay;\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 +2026,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 +2052,31 @@ 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> +\tif (ret != HTTP_OK) {\n> +\t\tif (retryable_code(http_code) && (retry_cnt < http_retry_limit)) {\n\nThe parentheses around the second condition should be dropped.\n\n> +\t\t\tretry_cnt++;\n> +\t\t\tretry_delay = http_retry_after(result_headers, retry_delay);\n> +\t\t\tfprintf(stderr,\n\nShould this be a `warning()` instead? I see 5 instances in `http.c` that\nuse `fprintf(stderr, ...)`, but 12 that use `warning()`, making me believe\nthat at least some of those 5 instances should call `warning()` instead,\ntoo.\n\n> +\t\t\t Q_(\"got HTTP response %d, retrying after %d second (%d/%d)\\n\",\n> +\t\t\t\t \"got HTTP response %d, retrying after %d seconds (%d/%d)\\n\",\n> +\t\t\t\t\tretry_delay),\n> +\t\t\t\thttp_code, retry_delay, retry_cnt, http_retry_limit);\n> +\t\t\tsleep(retry_delay);\n> +\n> +\t\t\t// remove header data fields since not all slots will use them\n\nNo C++-style comments, please: use /* ... */ instead.\n\n> +\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, NULL);\n> +\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, NULL);\n> +\n> +\t\t\tgoto retry;\n> +\t\t}\n> +\t}\n> +\n> +\t// remove header data fields since not all slots will use them\n\nNo C++-style comments, please.\n\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, NULL);\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, NULL);\n\nShouldn't we just perform this assignment before the `if (ret != HTTP_OK)`\ncondition? I do not see anything inside that block that needs it,\ntherefore this could be DRY'd up.\n\n>\n> \tif (options && options->content_type) {\n> \t\tstruct strbuf raw = STRBUF_INIT;\n> diff --git a/http.h b/http.h\n> index 5de792ef3f..60ce03801f 100644\n> --- a/http.h\n> +++ b/http.h\n> @@ -86,6 +86,17 @@ size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n> curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);\n> #endif\n>\n> +/*\n> + * Query the value of an HTTP header.\n> + *\n> + * If the header is found, then a newly allocate string is returned through\n> + * the value parameter, and the length is returned.\n> + *\n> + * If not found, returns 0\n> + */\n> +size_t http_header_value(\n> +\tconst struct strbuf headers, const char *header, char **value);\n\nDo we really need to export this function? It could stay file-local, at\nleast for now (i.e. be defined `static` inside `http.c`), no?\n\n> +\n> /* Slot lifecycle functions */\n> struct active_request_slot *get_active_slot(void);\n> int start_active_slot(struct active_request_slot *slot);\n> @@ -99,7 +110,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);\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 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;\n> diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh\n> index 82aa99ae87..e09083e2b3 100755\n> --- a/t/t5539-fetch-http-shallow.sh\n> +++ b/t/t5539-fetch-http-shallow.sh\n> @@ -30,20 +30,39 @@ test_expect_success 'clone http repository' '\n> \tgit clone --bare --no-local shallow \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> \tgit clone $HTTPD_URL/smart/repo.git clone &&\n> \t(\n> -\tcd clone &&\n> -\tgit fsck &&\n> -\tgit log --format=%s origin/master >actual &&\n> -\tcat <expect &&\n> -7\n> -6\n> -5\n> -4\n> -3\n> -EOF\n> -\ttest_cmp expect actual\n> +\t\tcd clone &&\n> +\t\tgit fsck &&\n> +\t\tgit log --format=%s origin/master >actual &&\n> +\t\tcat <<-\\EOF >expect &&\n> +\t\t7\n> +\t\t6\n> +\t\t5\n> +\t\t4\n> +\t\t3\n> +\t\tEOF\n> +\t\ttest_cmp expect actual\n\nThis just changes the indentation, right?\n\nI _guess_ this is a good change, but it should live in its own patch.\n\n> \t)\n> '\n>\n> +test_expect_success 'clone http repository with flaky http' '\n> + rm -rf clone &&\n\nLet's consistently use horizontal tab characters for indentation. (There\nare more instances of lines indented by spaces below.)\n\n> +\tgit clone $HTTPD_URL/error_ntime/`gen_nonce`/3/429/1/smart/repo.git clone 2>err &&\n\nLet's use `$(gen_nonce)`. Also: where is the `gen_nonce` defined? I do not\nsee the definition in this patch (but it could be 1/3, which for some\nreason did not make it to the mailing list:\nhttps://lore.kernel.org/git/20201012184806.166251-1-smcallis@google.com/).\n\nAnother suggestion: rather than deleting `clone/`, use a separate\ndirectory to clone into, say, `flaky/`. That will make it easier to debug\nwhen the entire \"trash\" directory is tar'ed up in a failed CI build, for\nexample.\n\n> +\t(\n> +\t\tcd clone &&\n> +\t\tgit fsck &&\n> +\t\tgit log --format=%s origin/master >actual &&\n> +\t\tcat <<-\\EOF >expect &&\n> +\t\t7\n> +\t\t6\n> +\t\t5\n> +\t\t4\n> +\t\t3\n> +\t\tEOF\n> +\t\ttest_cmp expect actual\n> +\t) &&\n> + test_i18ngrep \"got HTTP response 429\" err\n> +'\n> +\n> # This test is tricky. We need large enough \"have\"s that fetch-pack\n> # will put pkt-flush in between. Then we need a \"have\" the server\n> # does not have, it'll send \"ACK %s ready\"\n> diff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh\n> index 450321fddb..d8234d555c 100755\n> --- a/t/t5540-http-push-webdav.sh\n> +++ b/t/t5540-http-push-webdav.sh\n> @@ -68,12 +68,36 @@ test_expect_success 'push already up-to-date' '\n> \tgit push\n> '\n>\n> +test_expect_success 'push to remote repository with packed refs and flakey server' '\n> +\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n> +\t rm -rf packed-refs &&\n> +\t git update-ref refs/heads/master $ORIG_HEAD &&\n> +\t git --bare update-server-info) &&\n> + git remote set-url origin $HTTPD_URL/error_ntime/`gen_nonce`/3/502/1/dumb/test_repo.git &&\n> +\tgit push &&\n> + git remote set-url origin $HTTPD_URL/dumb/test_repo.git &&\n> +\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n> +\t test $HEAD = $(git rev-parse --verify HEAD))\n> +'\n> +\n> test_expect_success 'push to remote repository with unpacked refs' '\n> \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n> -\t rm packed-refs &&\n> +\t rm -rf packed-refs &&\n> +\t git update-ref refs/heads/master $ORIG_HEAD &&\n> +\t git --bare update-server-info) &&\n> +\tgit push &&\n> +\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n> +\t test $HEAD = $(git rev-parse --verify HEAD))\n> +'\n> +\n> +test_expect_success 'push to remote repository with unpacked refs and flakey server' '\n> +\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n> +\t rm -rf packed-refs &&\n> \t git update-ref refs/heads/master $ORIG_HEAD &&\n> \t git --bare update-server-info) &&\n> + git remote set-url origin $HTTPD_URL/error_ntime/`gen_nonce`/3/502/1/dumb/test_repo.git &&\n> \tgit push &&\n> + git remote set-url origin $HTTPD_URL/dumb/test_repo.git &&\n> \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n> \t test $HEAD = $(git rev-parse --verify HEAD))\n> '\n> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n> index 187454f5dd..9b2fc62e11 100755\n> --- a/t/t5541-http-push-smart.sh\n> +++ b/t/t5541-http-push-smart.sh\n> @@ -77,6 +77,21 @@ test_expect_success 'push to remote repository (standard)' '\n> \t test $HEAD = $(git rev-parse --verify HEAD))\n> '\n>\n> +test_expect_success 'push to remote repository (flakey server)' '\n> +\tcd \"$ROOT_PATH\"/test_repo_clone &&\n> +\t: >path5 &&\n> +\tgit add path5 &&\n> +\ttest_tick &&\n> +\tgit commit -m path5 &&\n> +\tHEAD=$(git rev-parse --verify HEAD) &&\n> +\tgit remote set-url origin $HTTPD_URL/error_ntime/`gen_nonce`/3/502/1/smart/test_repo.git &&\n> +\tGIT_TRACE_CURL=true git push -v -v 2>err &&\n> +\tgit remote set-url origin $HTTPD_URL/smart/test_repo.git &&\n> +\tgrep \"POST git-receive-pack ([0-9]* bytes)\" err &&\n> +\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git &&\n> +\ttest $HEAD = $(git rev-parse --verify HEAD))\n> +'\n> +\n> test_expect_success 'push already up-to-date' '\n> \tgit push\n> '\n> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\n> index 483578b2d7..350c47097b 100755\n> --- a/t/t5550-http-fetch-dumb.sh\n> +++ b/t/t5550-http-fetch-dumb.sh\n> @@ -176,6 +176,18 @@ test_expect_success 'fetch changes via http' '\n> \ttest_cmp file clone/file\n> '\n>\n> +test_expect_success 'fetch changes via flakey http' '\n> +\techo content >>file &&\n> +\tgit commit -a -m three &&\n> +\tgit push public &&\n> +\t(cd clone &&\n> +\t\tgit remote set-url origin $HTTPD_URL/error_ntime/`gen_nonce`/3/502/1/dumb/repo.git &&\n> +\t\tgit pull 2>../err &&\n> +\t\tgit remote set-url origin $HTTPD_URL/dumb/repo.git) &&\n> + test_i18ngrep \"got HTTP response 502\" err &&\n> +\ttest_cmp file clone/file\n> +'\n> +\n> test_expect_success 'fetch changes via manual http-fetch' '\n> \tcp -R clone-tmpl clone2 &&\n>\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index e40e9ed52f..85d2a0e8b8 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -45,6 +45,7 @@ test_expect_success 'clone http repository' '\n> \tEOF\n> \tGIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION=0 \\\n> \t\tgit clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n> + cd clone && git config pull.rebase false && cd .. &&\n\nBetter: test_config -C clone pull.rebase false\n\n> \ttest_cmp file clone/file &&\n> \ttr '\\''\\015'\\'' Q \tsed -e \"\n> @@ -103,6 +104,19 @@ test_expect_success 'fetch changes via http' '\n> \ttest_cmp file clone/file\n> '\n>\n> +test_expect_success 'fetch changes via flakey http' '\n> +\techo content >>file &&\n> +\tgit commit -a -m two &&\n> +\tgit push public &&\n> +\t(cd clone &&\n> +\t\tgit remote set-url origin $HTTPD_URL/error_ntime/`gen_nonce`/3/502/1/smart/repo.git &&\n> +\t\tgit pull 2>../err &&\n> +\t\tgit remote set-url origin $HTTPD_URL/smart/repo.git) &&\n> +\ttest_cmp file clone/file &&\n> + test_i18ngrep \"got HTTP response 502\" err\n> +'\n> +\n> +\n> test_expect_success 'used upload-pack service' '\n> \tcat >exp <<-\\EOF &&\n> \tGET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 71d4307001..9988e3ff14 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> - _NONCE=`gen_nonce` && export _NONCE &&\n> + _NONCE=`gen_nonce` &&\n> curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n> curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n> curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\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> + test_i18ngrep \"got HTTP response 429\" err\n> +'\n\nI wonder whether it is really necessary to add _that_ many test cases. The\ntest suite already takes so long to run that we had cases where\ncontributors simply did not run it before sending their contributions.\n\nIn this instance, I would think that it would be plenty sufficient to have\na single new test case that exercizes the added code path (and verifies\nthat it can see the message).\n\nThanks,\nDscho\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> 2.28.0.1011.ga647a8990f-goog\n>\n>\n"},{"id":"407384","messageId":"20201012201940.229694-1-smcallis@google.com","threadId":"54400","inReplyTo":"20201012184806.166251-1-smcallis@google.com","subject":"[PATCH] remote-curl: add testing for intelligent retry for HTTP","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-12T20:19:40Z","receivedAt":"2020-10-12T20:19:47Z","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 | 6 +++\n t/lib-httpd/apache.conf | 9 +++++\n t/lib-httpd/error-ntime.sh | 79 ++++++++++++++++++++++++++++++++++++++\n t/t5601-clone.sh | 9 +++++\n 4 files changed, 103 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..da1d4adfb4 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,8 @@ check_access_log() {\n \t\ttest_cmp \"$1\" access.log.stripped\n \tfi\n }\n+\n+# generate a random 12 digit string\n+gen_nonce() {\n+ test_copy_bytes 12 < /dev/urandom | tr -dc A-Za-z0-9\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..e4f91ab816\n--- /dev/null\n+++ b/t/lib-httpd/error-ntime.sh\n@@ -0,0 +1,79 @@\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+ if [ \"$1\" -eq \"302\" ]; then printf \"Status: 302 Found\\n\"\n+ elif [ \"$1\" -eq \"429\" ]; then printf \"Status: 429 Too Many Requests\\n\"\n+ elif [ \"$1\" -eq \"502\" ]; then printf \"Status: 502 Bad Gateway\\n\"\n+ elif [ \"$1\" -eq \"503\" ]; then printf \"Status: 503 Service Unavailable\\n\"\n+ elif [ \"$1\" -eq \"504\" ]; then 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+# read in path split into cmoponents\n+IFS='/'\n+tokens=($PATH_INFO)\n+\n+# break out code/retry parts of path\n+nonce=${tokens[1]}\n+times=${tokens[2]}\n+code=${tokens[3]}\n+retry=${tokens[4]}\n+\n+# get redirect path\n+cnt=0\n+path=\"\"\n+for ((ii=0; ii < ${#PATH_INFO}; ii++)); do\n+ if [ \"${PATH_INFO:${ii}:1}\" == \"/\" ]; then\n+ let cnt=${cnt}+1\n+ fi\n+ if [ \"${cnt}\" -eq 5 ]; then\n+ path=\"${PATH_INFO:${ii}}\"\n+ break\n+ fi\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+ echo 0 > \"$state_file\"\n+fi\n+\n+\n+read cnt < \"$state_file\"\n+if [ \"$cnt\" -lt \"$times\" ]; then\n+ let cnt=cnt+1\n+ echo \"$cnt\" > \"$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\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 7df3c5373a..71d4307001 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+ _NONCE=`gen_nonce` && export _NONCE &&\n+ curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n+ curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n+ curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\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":"407390","messageId":"xmqqy2kbmalb.fsf@gitster.c.googlers.com","threadId":"54400","inReplyTo":"20201012201940.229694-1-smcallis@google.com","subject":"Re: [PATCH] remote-curl: add testing for intelligent retry for HTTP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T20:51:44Z","receivedAt":"2020-10-12T20:51:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sean McAllister writes:\n\n> +# generate a random 12 digit string\n> +gen_nonce() {\n> + test_copy_bytes 12 < /dev/urandom | tr -dc A-Za-z0-9\n> +}\n\nWhat is the randomness requirement of this application? IOW, what\nbreaks if we just change this to \"echo 0123456789ab\"?\n\nOr \"date | git hash-object --stdin\" for that matter?\n\nWe'd want to make our tests more predictiable, not less.\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..e4f91ab816\n> --- /dev/null\n> +++ b/t/lib-httpd/error-ntime.sh\n> @@ -0,0 +1,79 @@\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> + if [ \"$1\" -eq \"302\" ]; then printf \"Status: 302 Found\\n\"\n> + elif [ \"$1\" -eq \"429\" ]; then printf \"Status: 429 Too Many Requests\\n\"\n> + elif [ \"$1\" -eq \"502\" ]; then printf \"Status: 502 Bad Gateway\\n\"\n> + elif [ \"$1\" -eq \"503\" ]; then printf \"Status: 503 Service Unavailable\\n\"\n> + elif [ \"$1\" -eq \"504\" ]; then 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\nStyle????? (I won't repeat this comment for the rest of this script)\n\nI briefly wondered \"oh, are t/lib-httpd/* scripts excempt from the\ncoding guidelines?\" but a quick look at them tells me that that is\nnot the case.\n\n> +}\n> +\n> +# read in path split into cmoponents\n> +IFS='/'\n> +tokens=($PATH_INFO)\n> +\n> +# break out code/retry parts of path\n> +nonce=${tokens[1]}\n> +times=${tokens[2]}\n> +code=${tokens[3]}\n> +retry=${tokens[4]}\n\nYou said /bin/sh upfront. Don't use non-POSIX shell arrays.\n\n> +\n> +# get redirect path\n> +cnt=0\n> +path=\"\"\n> +for ((ii=0; ii < ${#PATH_INFO}; ii++)); do\n> + if [ \"${PATH_INFO:${ii}:1}\" == \"/\" ]; then\n> + let cnt=${cnt}+1\n> + fi\n> + if [ \"${cnt}\" -eq 5 ]; then\n> + path=\"${PATH_INFO:${ii}}\"\n> + break\n> + fi\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> + echo 0 > \"$state_file\"\n> +fi\n> +\n> +\n> +read cnt < \"$state_file\"\n> +if [ \"$cnt\" -lt \"$times\" ]; then\n> + let cnt=cnt+1\n> + echo \"$cnt\" > \"$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> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 7df3c5373a..71d4307001 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> + _NONCE=`gen_nonce` && export _NONCE &&\n> + curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n> + curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n> + curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n\nThese lines are not indented with HT?\n\nDon't redirect test output to /dev/null, which is done by test_expect_success\nfor us. >/dev/null makes it less useful to run the test under \"-v\" option.\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":"407393","messageId":"xmqqtuuzma6l.fsf@gitster.c.googlers.com","threadId":"54400","inReplyTo":"nycvar.QRO.7.76.6.2010122126280.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/3] http: automatically retry some requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T21:00:34Z","receivedAt":"2020-10-12T21:00:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Sean,\n>\n> On Mon, 12 Oct 2020, 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>\n> Makes a lot of sense to me.\n> ...\n>> +http.retryLimit::\n>> +\tSome HTTP error codes (eg: 429,503) can reasonably be retried if\n>\n> Please have a space after the comma so that it is not being mistaken for a\n> 6-digit number. Also, aren't they called \"status codes\"? Not all of them\n> indicate errors, after all.\n> ...\n\nI've read your comments and agree to them all. Thanks for a\ndetailed and excellent review.\n\n"},{"id":"407397","messageId":"CAM4o00e4wYOHkn38H8UwqboRMSzAs4QCvTN6Ef6PuUnYfwOoXg@mail.gmail.com","threadId":"54400","inReplyTo":"xmqqy2kbmalb.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] remote-curl: add testing for intelligent retry for HTTP","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-12T22:20:49Z","receivedAt":"2020-10-12T22:21:03Z","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 random 12 digit string\n> > +gen_nonce() {\n> > + test_copy_bytes 12 < /dev/urandom | tr -dc A-Za-z0-9\n> > +}\n>\n> What is the randomness requirement of this application? IOW, what\n> breaks if we just change this to \"echo 0123456789ab\"?\n>\n> Or \"date | git hash-object --stdin\" for that matter?\n>\n> We'd want to make our tests more predictiable, not less.\n\nThe randomness requirement is just that I need nonces to be unique\nduring a single run of the HTTP server\nas they uniquefy the files I put on disk to make the HTTP hack-ily\nstateful. I'd be fine with your date/hash-object\nsolution, but I don't know that it will help make the tests more predictable.\n\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..e4f91ab816\n> > --- /dev/null\n> > +++ b/t/lib-httpd/error-ntime.sh\n> > @@ -0,0 +1,79 @@\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> > + if [ \"$1\" -eq \"302\" ]; then printf \"Status: 302 Found\\n\"\n> > + elif [ \"$1\" -eq \"429\" ]; then printf \"Status: 429 Too Many Requests\\n\"\n> > + elif [ \"$1\" -eq \"502\" ]; then printf \"Status: 502 Bad Gateway\\n\"\n> > + elif [ \"$1\" -eq \"503\" ]; then printf \"Status: 503 Service Unavailable\\n\"\n> > + elif [ \"$1\" -eq \"504\" ]; then 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> Style????? (I won't repeat this comment for the rest of this script)\n>\n> I briefly wondered \"oh, are t/lib-httpd/* scripts excempt from the\n> coding guidelines?\" but a quick look at them tells me that that is\n> not the case.\n>\n\nI mistakenly thought the Makefile in t/ was linting these as well.\nI've gone back through and fixed formatting issues and removed\nnon-posix constructs.\n\n> > +}\n> > +\n> > +# read in path split into cmoponents\n> > +IFS='/'\n> > +tokens=($PATH_INFO)\n> > +\n> > +# break out code/retry parts of path\n> > +nonce=${tokens[1]}\n> > +times=${tokens[2]}\n> > +code=${tokens[3]}\n> > +retry=${tokens[4]}\n>\n> You said /bin/sh upfront. Don't use non-POSIX shell arrays.\n>\n> > +\n> > +# get redirect path\n> > +cnt=0\n> > +path=\"\"\n> > +for ((ii=0; ii < ${#PATH_INFO}; ii++)); do\n> > + if [ \"${PATH_INFO:${ii}:1}\" == \"/\" ]; then\n> > + let cnt=${cnt}+1\n> > + fi\n> > + if [ \"${cnt}\" -eq 5 ]; then\n> > + path=\"${PATH_INFO:${ii}}\"\n> > + break\n> > + fi\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> > + echo 0 > \"$state_file\"\n> > +fi\n> > +\n> > +\n> > +read cnt < \"$state_file\"\n> > +if [ \"$cnt\" -lt \"$times\" ]; then\n> > + let cnt=cnt+1\n> > + echo \"$cnt\" > \"$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> > diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> > index 7df3c5373a..71d4307001 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\" > /dev/null &&\n> > + curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n> > + curl \"$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server\" > /dev/null &&\n>\n> These lines are not indented with HT?\n>\n> Don't redirect test output to /dev/null, which is done by test_expect_success\n> for us. >/dev/null makes it less useful to run the test under \"-v\" option.\n>\n\nFixed in v2.\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":"407400","messageId":"xmqqd01nm60u.fsf@gitster.c.googlers.com","threadId":"54400","inReplyTo":"CAM4o00e4wYOHkn38H8UwqboRMSzAs4QCvTN6Ef6PuUnYfwOoXg@mail.gmail.com","subject":"Re: [PATCH] remote-curl: add testing for intelligent retry for HTTP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T22:30:25Z","receivedAt":"2020-10-12T22:30:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sean McAllister writes:\n\n>> Sean McAllister writes:\n>>\n>> > +# generate a random 12 digit string\n>> > +gen_nonce() {\n>> > + test_copy_bytes 12 < /dev/urandom | tr -dc A-Za-z0-9\n>> > +}\n>>\n>> What is the randomness requirement of this application? IOW, what\n>> breaks if we just change this to \"echo 0123456789ab\"?\n>>\n>> Or \"date | git hash-object --stdin\" for that matter?\n>>\n>> We'd want to make our tests more predictiable, not less.\n>\n> The randomness requirement is just that I need nonces to be unique\n> during a single run of the HTTP server\n> as they uniquefy the files I put on disk to make the HTTP hack-ily\n> stateful. I'd be fine with your date/hash-object\n> solution, but I don't know that it will help make the tests more predictable.\n\nIf so, would something like this be\n\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\nmore appropriate? It is utterly predictable and yields the same\nanswer only once during a single run.\n"},{"id":"407446","messageId":"nycvar.QRO.7.76.6.2010131624060.50@tvgsbejvaqbjf.bet","threadId":"54400","inReplyTo":"xmqqd01nm60u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] remote-curl: add testing for intelligent retry for HTTP","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-13T14:25:11Z","receivedAt":"2020-10-13T14:25:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 12 Oct 2020, Junio C Hamano wrote:\n\n> Sean McAllister writes:\n>\n> >> Sean McAllister writes:\n> >>\n> >> > +# generate a random 12 digit string\n> >> > +gen_nonce() {\n> >> > + test_copy_bytes 12 < /dev/urandom | tr -dc A-Za-z0-9\n> >> > +}\n> >>\n> >> What is the randomness requirement of this application? IOW, what\n> >> breaks if we just change this to \"echo 0123456789ab\"?\n> >>\n> >> Or \"date | git hash-object --stdin\" for that matter?\n> >>\n> >> We'd want to make our tests more predictiable, not less.\n> >\n> > The randomness requirement is just that I need nonces to be unique\n> > during a single run of the HTTP server\n> > as they uniquefy the files I put on disk to make the HTTP hack-ily\n> > stateful. I'd be fine with your date/hash-object\n> > solution, but I don't know that it will help make the tests more predictable.\n>\n> If so, would something like this be\n>\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>\n> more appropriate? It is utterly predictable and yields the same\n> answer only once during a single run.\n\nWe should also consider using `test-tool genrandom ` instead (where\n`` would have to be predictable, but probably would have to change\nbetween `gen_nonce()` calls).\n\nCiao,\nDscho\n"},{"id":"407451","messageId":"CAM4o00fL4oGNG_Z7tF5bL=Kp===683LBo1RhmZ=vZ6Kie=-jzA@mail.gmail.com","threadId":"54400","inReplyTo":"nycvar.QRO.7.76.6.2010122126280.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/3] http: automatically retry some requests","fromName":"Sean McAllister","fromEmail":"smcallis@google.com","sentAt":"2020-10-13T15:03:15Z","receivedAt":"2020-10-13T15:03:30Z","isPatch":true,"sender":{"key":"smcallis@google.com","avatar":"https://gravatar.com/avatar/a7d1fc1441a45a45c405b8cb462173e3ff1f7d5dd76916a6b8d10905a1c2e770?d=mp&s=160"},"body":"> >\n> > +http.retryLimit::\n> > + Some HTTP error codes (eg: 429,503) can reasonably be retried if\n>\n> Please have a space after the comma so that it is not being mistaken for a\n> 6-digit number. Also, aren't they called \"status codes\"? Not all of them\n> indicate errors, after all.\n\nDone for v2, good point on the nomenclature.\n\n> > diff --git a/http.c b/http.c\n> > index b3c1669388..e41d7c5575 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 = 2;\n>\n> Should there be a config option for that? Also, it took me some time to\n> find the code using this variable in order to find out what unit to use:\n> it is seconds (not microseconds, as I had expected). Maybe this can be\n> documented in the variable name, or at least in a comment?\n>\n\nJunio tossed that out during our private review and I think we decided to just\nleave them as non-const so we have that option going forward. I'm not\nsure there's\na strong story for configuring the default delay.\n\nI went through and changed all the _delay variables to be more explicit they're\nin seconds.\n\n> > @@ -219,6 +222,51 @@ size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)\n> > return nmemb;\n> > }\n> >\n> > +\n> > +/* return 1 for a retryable HTTP code, 0 otherwise */\n> > +static int retryable_code(int code)\n> > +{\n> > + switch(code) {\n> > + case 429: /* fallthrough */\n> > + case 502: /* fallthrough */\n> > + case 503: /* fallthrough */\n> > + case 504: return 1;\n> > + default: return 0;\n> > + }\n> > +}\n> > +\n> > +size_t http_header_value(\n>\n> In the part of Git's code with which I am familiar, we avoid trying to\n> break the line after an opening parenthesis, instead preferring to break\n> after a comma.\n>\n> Also, shouldn't we make the return type `ssize_t` to allow for a negative\n> value to indicate an error/missing header?\n\nI return zero to indicate no header found (or zero-length value), so this should\nnever return a negative value. One can check value to see if a string\nwas allocated\nfor a zero-length header.\n\n>\n> > + const struct strbuf headers, const char *header, char **value)\n> > +{\n> > + size_t len = 0;\n> > + struct strbuf **lines, **line;\n> > + char *colon = NULL;\n> > +\n> > + lines = strbuf_split(&headers, '\\n');\n> > + for (line = lines; *line; line++) {\n> > + strbuf_trim(*line);\n> > +\n> > + /* find colon and null it out to 'split' string */\n> > + colon = strchr((*line)->buf, ':');\n> > + if (colon) {\n> > + *colon = '\\0';\n> > +\n> > + if (!strcasecmp(header, (*line)->buf)) {\n>\n> If all we want is to find the given header, splitting lines seems to be a\n> bit wasteful to me. We could instead search for the header directly:\n>\n> const char *p = strcasestr(headers.buf, header), *eol;\n> size_t header_len = strlen(header);\n>\n> while (p) {\n> if ((p != headers.buf && p[-1] != '\\n') ||\n> p[header_len] != ':') {\n> p = strcasestr(p + header_len, header);\n> continue;\n> }\n>\n> p += header_len + 1;\n> while (isspace(*p) && *p != '\\n')\n> p++;\n> eol = strchrnul(p, '\\n');\n> len = eol - p;\n> *value = xstrndup(p, len);\n> return len;\n> }\n>\n\nI've been writing a lot of python code lately =D So splitting into\nlines was a natural paradigm for me. You're right, I like yours more. I've\nrefactored it to be closer to that. Little bit of fiddling to deal with header\nwhitespace properly, but it's pretty close.\n\nI also modified to just return the value pointer directly, then it's clear when\nwe get a zero-length header or don't find it completely, and it fixes the\nleak issue below.\n\n> > @@ -1903,20 +1958,55 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)\n> > #define HTTP_REQUEST_STRBUF 0\n> > #define HTTP_REQUEST_FILE 1\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 delay\n> > +*/\n>\n> Multi-line comments should be of this form:\n>\n> /*\n> * Check for a retry-after header in the given headers string, if\n> * found, then honor it, otherwise do an exponential backoff up to\n> * the maximum on the current delay.\n> */\n>\nDone.\n\n> > +static int http_retry_after(const struct strbuf headers, int cur_delay) {\n>\n> For functions, the initial opening curly bracket goes on its own line.\n>\nDone.\n\n> > + int len, delay;\n> > + char *end;\n> > + char *value;\n>\n> Why not declare `char *end, *value;`, just like `len` and `delay` are\n> declared on the same line?\n>\nDone.\n\n> > +\n> > + len = http_header_value(headers, \"retry-after\", &value);\n> > + if (len) {\n> > + delay = strtol(value, &end, 0);\n> > + if (*value && *end == 0 && delay >= 0) {\n>\n> Better: `*end == '\\0'`\n>\n> And why `*value` here? We already called `strtol()` on it.\n>\nFixed, and I check value per the man page on strtol:\n > In particular, if *nptr is not '\\0' but **endptr is '\\0' on\nreturn, the entire string is valid.\nI wanted to make sure I converted the entire header value, so this\nseemed correct?\n\n> > + if (delay > http_max_delay) {\n> > + die(Q_(\n>\n> Let's not end the line in an opening parenthesis. Instead, use C's string\n> continuation like so:\n>\n> die(Q_(\"server requested retry after %d second,\"\n> \" which is longer than max allowed\\n\",\n> \"server requested retry after %d \"\n> \"seconds, which is longer than max \"\n> \"allowed\\n\", delay), delay);\n>\nDone.\n\n> > + \"server requested retry after %d second, which is longer than max allowed\\n\",\n> > + \"server requested retry after %d seconds, which is longer than max allowed\\n\", delay), delay);\n> > + }\n> > + free(value);\n>\n> `value` is not actually used after that `strtol()` call above, so let's\n> release it right then and there.\n>\nGood call, done.\n\n> > + return delay;\n> > + }\n> > + free(value);\n> > + }\n>\n> If the header was found, but for some reason had an empty value, we're\n> leaking `value` here.\n>\nI return the pointer directly and check that now, so if it's allocated, we'll\nalways call free, even if it's an empty string.\n\n> > +\n> > + cur_delay *= 2;\n> > + return cur_delay >= http_max_delay ? http_max_delay : cur_delay;\n> > +}\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> > const char *accept_language;\n> > int ret;\n> > + int retry_cnt = 0;\n> > + int retry_delay = http_default_delay;\n> > + int http_code;\n> >\n> > +retry:\n> > slot = get_active_slot();\n> > curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n> >\n> > + curl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, &result_headers);\n> > + curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_buffer);\n> > +\n> > if (result == NULL) {\n> > curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);\n> > } else {\n> > @@ -1936,6 +2026,7 @@ static int http_request(const char *url,\n> >\n> > accept_language = get_accept_language();\n> >\n> > + headers = http_copy_default_headers();\n> > if (accept_language)\n> > headers = curl_slist_append(headers, accept_language);\n> >\n> > @@ -1961,7 +2052,31 @@ static int http_request(const char *url,\n> > curl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n> > curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n> >\n> > - ret = run_one_slot(slot, &results);\n> > + http_code = 0;\n> > + ret = run_one_slot(slot, &results, &http_code);\n> > +\n> > + if (ret != HTTP_OK) {\n> > + if (retryable_code(http_code) && (retry_cnt < http_retry_limit)) {\n>\n> The parentheses around the second condition should be dropped.\n>\nDone.\n\n> > + retry_cnt++;\n> > + retry_delay = http_retry_after(result_headers, retry_delay);\n> > + fprintf(stderr,\n>\n> Should this be a `warning()` instead? I see 5 instances in `http.c` that\n> use `fprintf(stderr, ...)`, but 12 that use `warning()`, making me believe\n> that at least some of those 5 instances should call `warning()` instead,\n> too.\n>\nAt your discretion. I'm not familiar with the logging framework.\nOnly one of those 5 fprintf\nis in my patch, so I changed that one to use warning().\n\n> > + Q_(\"got HTTP response %d, retrying after %d second (%d/%d)\\n\",\n> > + \"got HTTP response %d, retrying after %d seconds (%d/%d)\\n\",\n> > + retry_delay),\n> > + http_code, retry_delay, retry_cnt, http_retry_limit);\n> > + sleep(retry_delay);\n> > +\n> > + // remove header data fields since not all slots will use them\n>\n> No C++-style comments, please: use /* ... */ instead.\n>\nThought I got them all. Done.\n\n> > + curl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, NULL);\n> > + curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, NULL);\n> > +\n> > + goto retry;\n> > + }\n> > + }\n> > +\n> > + // remove header data fields since not all slots will use them\n>\n> No C++-style comments, please.\n>\nDone.\n\n> > + curl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, NULL);\n> > + curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, NULL);\n>\n> Shouldn't we just perform this assignment before the `if (ret != HTTP_OK)`\n> condition? I do not see anything inside that block that needs it,\n> therefore this could be DRY'd up.\n>\nExcellent idea, done.\n\n> > +/*\n> > + * Query the value of an HTTP header.\n> > + *\n> > + * If the header is found, then a newly allocate string is returned through\n> > + * the value parameter, and the length is returned.\n> > + *\n> > + * If not found, returns 0\n> > + */\n> > +size_t http_header_value(\n> > + const struct strbuf headers, const char *header, char **value);\n>\n> Do we really need to export this function? It could stay file-local, at\n> least for now (i.e. be defined `static` inside `http.c`), no?\n>\nI originally thought I'd need it elsewhere, so a big of a relic there. I made\nit static for now.\n\n> > --- a/t/t5539-fetch-http-shallow.sh\n> > +++ b/t/t5539-fetch-http-shallow.sh\n> > @@ -30,20 +30,39 @@ test_expect_success 'clone http repository' '\n> > git clone --bare --no-local shallow \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> > git clone $HTTPD_URL/smart/repo.git clone &&\n> > (\n> > - cd clone &&\n> > - git fsck &&\n> > - git log --format=%s origin/master >actual &&\n> > - cat <expect &&\n> > -7\n> > -6\n> > -5\n> > -4\n> > -3\n> > -EOF\n> > - test_cmp expect actual\n> > + cd clone &&\n> > + git fsck &&\n> > + git log --format=%s origin/master >actual &&\n> > + cat <<-\\EOF >expect &&\n> > + 7\n> > + 6\n> > + 5\n> > + 4\n> > + 3\n> > + EOF\n> > + test_cmp expect actual\n>\n> This just changes the indentation, right?\n>\n> I _guess_ this is a good change, but it should live in its own patch.\n>\nThis was in combination with my adding a new test, we cleaned up the\nwhitespace too per Junio rather than copy-and-pasting ill formatted code.\n\nIt's OBE now because this change has been removed in v2.\n\n> > +test_expect_success 'clone http repository with flaky http' '\n> > + rm -rf clone &&\n>\n> Let's consistently use horizontal tab characters for indentation. (There\n> are more instances of lines indented by spaces below.)\n>\n*sigh* thought I got them all, fixed.\n\n> > + git clone $HTTPD_URL/error_ntime/`gen_nonce`/3/429/1/smart/repo.git clone 2>err &&\n>\n> Let's use `$(gen_nonce)`. Also: where is the `gen_nonce` defined? I do not\n> see the definition in this patch (but it could be 1/3, which for some\n> reason did not make it to the mailing list:\n> https://lore.kernel.org/git/20201012184806.166251-1-smcallis@google.com/).\n>\nDone, and it is indeed in 1/3 which got caught by the spam filter\n(should be available now).\n\n> Another suggestion: rather than deleting `clone/`, use a separate\n> directory to clone into, say, `flaky/`. That will make it easier to debug\n> when the entire \"trash\" directory is tar'ed up in a failed CI build, for\n> example.\n>\nDone.\n\n> > diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> > index e40e9ed52f..85d2a0e8b8 100755\n> > --- a/t/t5551-http-fetch-smart.sh\n> > +++ b/t/t5551-http-fetch-smart.sh\n> > @@ -45,6 +45,7 @@ test_expect_success 'clone http repository' '\n> > EOF\n> > GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION=0 \\\n> > git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n> > + cd clone && git config pull.rebase false && cd .. &&\n>\n> Better: test_config -C clone pull.rebase false\n>\nDone, but also OBE by removing tests.\n\n>\n> I wonder whether it is really necessary to add _that_ many test cases. The\n> test suite already takes so long to run that we had cases where\n> contributors simply did not run it before sending their contributions.\n>\n> In this instance, I would think that it would be plenty sufficient to have\n> a single new test case that exercizes the added code path (and verifies\n> that it can see the message).\n>\nDone, down to a single test (clone) that should exercise everything.\n\n> Thanks,\n> Dscho\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> > 2.28.0.1011.ga647a8990f-goog\n> >\n> >\n"},{"id":"407462","messageId":"xmqq7drukp9y.fsf@gitster.c.googlers.com","threadId":"54400","inReplyTo":"nycvar.QRO.7.76.6.2010131624060.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] remote-curl: add testing for intelligent retry for HTTP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T17:29:45Z","receivedAt":"2020-10-13T17:29:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> We should also consider using `test-tool genrandom ` instead (where\n> `` would have to be predictable, but probably would have to change\n> between `gen_nonce()` calls).\n\nYup, that is exactly why I asked Sean about randomness requirement.\n\nIt turns out that they care only about uniqueness, so the comparison\nis between keeping an ever-incrementing counter and (1) echoing its\ncurrent contents and/or (2) feeding it to \"test-tool genrandom\" as\nthe seed. The complexity of the code _we_ need to write anew is the\nsame, but echo would probably be a win in both the number of forks\nand cycles departments.\n\nThanks.\n"},{"id":"407463","messageId":"xmqq362ikoki.fsf@gitster.c.googlers.com","threadId":"54400","inReplyTo":"CAM4o00fL4oGNG_Z7tF5bL=Kp===683LBo1RhmZ=vZ6Kie=-jzA@mail.gmail.com","subject":"Re: [PATCH 3/3] http: automatically retry some requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T17:45:01Z","receivedAt":"2020-10-13T17:45:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sean McAllister writes:\n\n>> > +static int http_retry_limit = 3;\n>> > +static int http_default_delay = 2;\n>>\n>> Should there be a config option for that? Also, it took me some time to\n>> find the code using this variable in order to find out what unit to use:\n>> it is seconds (not microseconds, as I had expected). Maybe this can be\n>> documented in the variable name, or at least in a comment?\n>\n> Junio tossed that out during our private review and I think we decided to just\n\nNeeds clarification. Here \"that\" in \"tossed that out\" only refers\nto \"static int const http_retry_limit = 3\" and friends and nothing\nelse. There weren't any discussion on units or comments. I did\nmention that it is an obvious future possibility to make these\nconfigurable and that was why I suggested to \"toss out\" the const.\n\nIt seems we'll see names with \"seconds\" in them somewhere, which is\ngood.\n\n> I've been writing a lot of python code lately =D So splitting into\n> lines was a natural paradigm for me. You're right, I like yours more. I've\n> refactored it to be closer to that. Little bit of fiddling to deal with header\n> whitespace properly, but it's pretty close.\n\nGood. I personally think strbuf_split() is a mistaken API whose use\nneeds to be killed, so it makes me happy to see one new callsite we\ndidn't have to add ;-)\n\nThanks.\n"}]}