[PATCH] http: add a config to limit the connection time

[PATCH] http: add a config to limit the connection time

[GalaxySnail via GitGitGadget]

From: GalaxySnail <me@glxys.nl>

By default, libcurl uses a 300 seconds timeout for the connection phase,
which is too long for some use cases.

Add http.connecttimeoutms and GIT_HTTP_CONNECT_TIMEOUT_MS to specify
timeout in milliseconds for the connection phase. Both of them call
CURLOPT_CONNECTTIMEOUT_MS internally.

Signed-off-by: GalaxySnail <me@glxys.nl>
---
    http: add a config to limit the connection time

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2362%2FGalaxySnail%2Fhttp-connect-timeout-ms-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2362/GalaxySnail/http-connect-timeout-ms-v1
Pull-Request: https://github.com/git/git/pull/2362

 Documentation/config/http.adoc  |  7 ++++
 http.c                          | 11 ++++++
 t/meson.build                   |  1 +
 t/t5585-http-connect-timeout.sh | 60 +++++++++++++++++++++++++++++++++
 4 files changed, 79 insertions(+)
 create mode 100755 t/t5585-http-connect-timeout.sh

diff --git a/Documentation/config/http.adoc b/Documentation/config/http.adoc
index 792a71b413..a4f7afa61e 100644
--- a/Documentation/config/http.adoc
+++ b/Documentation/config/http.adoc
@@ -300,6 +300,13 @@ for most push problems, but can increase memory consumption
 significantly since the entire buffer is allocated even for small
 pushes.
 
+http.connectTimeoutMS::
+	Maximum time in milliseconds that you allow the connection phase
+	to take. The connection phase includes DNS lookup and subsequent
+	TCP, TLS or QUIC handshakes.
+	Can be overridden by the `GIT_HTTP_CONNECT_TIMEOUT_MS`
+	environment variable.
+
 http.lowSpeedLimit::
 http.lowSpeedTime::
 	If the HTTP transfer speed, in bytes per second, is less than
diff --git a/http.c b/http.c
index caccf2108e..befe9ea8a0 100644
--- a/http.c
+++ b/http.c
@@ -68,6 +68,7 @@ static char *ssl_capath;
 static char *curl_no_proxy;
 static char *ssl_pinnedkey;
 static char *ssl_cainfo;
+static long curl_connect_timeout_ms = -1;
 static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
@@ -450,6 +451,10 @@ static int http_options(const char *var, const char *value,
 		max_requests = git_config_int(var, value, ctx->kvi);
 		return 0;
 	}
+	if (!strcmp("http.connecttimeoutms", var)) {
+		curl_connect_timeout_ms = git_config_int(var, value, ctx->kvi);
+		return 0;
+	}
 	if (!strcmp("http.lowspeedlimit", var)) {
 		curl_low_speed_limit = git_config_int(var, value, ctx->kvi);
 		return 0;
@@ -1215,6 +1220,10 @@ static CURL *get_curl_handle(void)
 			curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, http_proxy_ssl_ca_info);
 	}
 
+	if (curl_connect_timeout_ms > 0)
+		curl_easy_setopt(result, CURLOPT_CONNECTTIMEOUT_MS,
+				 curl_connect_timeout_ms);
+
 	if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
 		curl_easy_setopt(result, CURLOPT_LOW_SPEED_LIMIT,
 				 curl_low_speed_limit);
@@ -1474,6 +1483,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 
 	set_from_env(&user_agent, "GIT_HTTP_USER_AGENT");
 
+	set_long_from_env(&curl_connect_timeout_ms, "GIT_HTTP_CONNECT_TIMEOUT_MS");
+
 	set_long_from_env(&curl_low_speed_limit, "GIT_HTTP_LOW_SPEED_LIMIT");
 	set_long_from_env(&curl_low_speed_time, "GIT_HTTP_LOW_SPEED_TIME");
 
diff --git a/t/meson.build b/t/meson.build
index 8ae6ab6c5f..6196736cb2 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -724,6 +724,7 @@ integration_tests = [
   't5582-fetch-negative-refspec.sh',
   't5583-push-branches.sh',
   't5584-http-429-retry.sh',
+  't5585-http-connect-timeout.sh',
   't5600-clone-fail-cleanup.sh',
   't5601-clone.sh',
   't5602-clone-remote-exec.sh',
diff --git a/t/t5585-http-connect-timeout.sh b/t/t5585-http-connect-timeout.sh
new file mode 100755
index 0000000000..7363e23bfe
--- /dev/null
+++ b/t/t5585-http-connect-timeout.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='test http.connecttimeoutms and GIT_HTTP_CONNECT_TIMEOUT_MS'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repository' '
+	test_commit initial &&
+	git clone --bare . "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" config http.receivepack true
+'
+
+test_expect_success 'http.connecttimeoutms accepts a positive integer via config' '
+	test_config http.connecttimeoutms 5000 &&
+	git ls-remote "$HTTPD_URL/smart/repo.git" >output &&
+	test_grep "refs/heads/" output
+'
+
+test_expect_success 'http.connecttimeoutms=0 is accepted (disables the option)' '
+	test_config http.connecttimeoutms 0 &&
+	git ls-remote "$HTTPD_URL/smart/repo.git" >output &&
+	test_grep "refs/heads/" output
+'
+
+test_expect_success 'GIT_HTTP_CONNECT_TIMEOUT_MS env var is accepted' '
+	GIT_HTTP_CONNECT_TIMEOUT_MS=5000 \
+		git ls-remote "$HTTPD_URL/smart/repo.git" >output 2>err &&
+	test_grep "refs/heads/" output &&
+	test_grep ! . err
+'
+
+test_expect_success 'http.connecttimeoutms rejects non-numeric config value' '
+	test_config http.connecttimeoutms not-a-number &&
+	test_must_fail git ls-remote "$HTTPD_URL/smart/repo.git" 2>err &&
+	test_grep "bad numeric config value .not-a-number. for .http\.connecttimeoutms." err
+'
+
+test_expect_success 'http.connecttimeoutms rejects empty config value' '
+	test_config http.connecttimeoutms "" &&
+	test_must_fail git ls-remote "$HTTPD_URL/smart/repo.git" 2>err &&
+	test_grep "bad numeric config value" err
+'
+
+test_expect_success 'GIT_HTTP_CONNECT_TIMEOUT_MS warns on non-numeric value but succeeds' '
+	GIT_HTTP_CONNECT_TIMEOUT_MS=not-a-number \
+		git ls-remote "$HTTPD_URL/smart/repo.git" >output 2>err &&
+	test_grep "refs/heads/" output &&
+	test_grep "failed to parse GIT_HTTP_CONNECT_TIMEOUT_MS" err
+'
+
+test_expect_success 'GIT_HTTP_CONNECT_TIMEOUT_MS warns on empty value but succeeds' '
+	GIT_HTTP_CONNECT_TIMEOUT_MS= \
+		git ls-remote "$HTTPD_URL/smart/repo.git" >output 2>err &&
+	test_grep "refs/heads/" output &&
+	test_grep "failed to parse GIT_HTTP_CONNECT_TIMEOUT_MS" err
+'
+
+test_done

base-commit: 9a0c4701dcd5725c4184599322b52933ff5005ca
-- 
gitgitgadget

Re: [PATCH] http: add a config to limit the connection time

[Junio C Hamano]

"GalaxySnail via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: GalaxySnail <me@glxys.nl>
>
> By default, libcurl uses a 300 seconds timeout for the connection phase,
> which is too long for some use cases.

Can you elaborate a bit more on the use cases in which you want to
try connecting to an unreachable host yet want to give up on it very
fast?

> Add http.connecttimeoutms and GIT_HTTP_CONNECT_TIMEOUT_MS to specify
> timeout in milliseconds for the connection phase. Both of them call
> CURLOPT_CONNECTTIMEOUT_MS internally.
>
> Signed-off-by: GalaxySnail <me@glxys.nl>

Documentation/SubmittingPatches:[[real-name]] applies here.

>  Documentation/config/http.adoc  |  7 ++++
>  http.c                          | 11 ++++++
>  t/meson.build                   |  1 +
>  t/t5585-http-connect-timeout.sh | 60 +++++++++++++++++++++++++++++++++
>  4 files changed, 79 insertions(+)
>  create mode 100755 t/t5585-http-connect-timeout.sh
>
> diff --git a/Documentation/config/http.adoc b/Documentation/config/http.adoc
> index 792a71b413..a4f7afa61e 100644
> --- a/Documentation/config/http.adoc
> +++ b/Documentation/config/http.adoc
> @@ -300,6 +300,13 @@ for most push problems, but can increase memory consumption
>  significantly since the entire buffer is allocated even for small
>  pushes.
>  
> +http.connectTimeoutMS::
> +	Maximum time in milliseconds that you allow the connection phase
> +	to take. The connection phase includes DNS lookup and subsequent
> +	TCP, TLS or QUIC handshakes.
> +	Can be overridden by the `GIT_HTTP_CONNECT_TIMEOUT_MS`
> +	environment variable.

Once a knob is provided, users will want to know what value is
used when unspecified, so they can gauge what a reasonable value to
set would be.

> diff --git a/http.c b/http.c
> index caccf2108e..befe9ea8a0 100644
> --- a/http.c
> +++ b/http.c
> @@ -68,6 +68,7 @@ static char *ssl_capath;
>  static char *curl_no_proxy;
>  static char *ssl_pinnedkey;
>  static char *ssl_cainfo;
> +static long curl_connect_timeout_ms = -1;
>  static long curl_low_speed_limit = -1;
>  static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv;
> @@ -450,6 +451,10 @@ static int http_options(const char *var, const char *value,
>  		max_requests = git_config_int(var, value, ctx->kvi);
>  		return 0;
>  	}
> +	if (!strcmp("http.connecttimeoutms", var)) {
> +		curl_connect_timeout_ms = git_config_int(var, value, ctx->kvi);
> +		return 0;
> +	}

We could set it to -1 if we wanted to, and behave as if no
configuration variable were given.  That may be reasonable, but it
should be documented.

> @@ -1215,6 +1220,10 @@ static CURL *get_curl_handle(void)
>  			curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, http_proxy_ssl_ca_info);
>  	}
>  
> +	if (curl_connect_timeout_ms > 0)
> +		curl_easy_setopt(result, CURLOPT_CONNECTTIMEOUT_MS,
> +				 curl_connect_timeout_ms);

This code silently ignores setting the configuration variable to 0.
To the cURL library, however, passing a value of 0 to
CURLOPT_CONNECTTIMEOUT_MS signals that it should use the default
value (300s).

Perhaps we should tweak the above to

	if (0 <= curlopt_connecttimeout_ms)
		curl_easy_setopt(result, CURLOPT_CONNECTTIMEOUT_MS,
				 curl_connect_timeout_ms);

and then document what 0 means.

> @@ -1474,6 +1483,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>  
>  	set_from_env(&user_agent, "GIT_HTTP_USER_AGENT");
>  
> +	set_long_from_env(&curl_connect_timeout_ms, "GIT_HTTP_CONNECT_TIMEOUT_MS");
> +
>  	set_long_from_env(&curl_low_speed_limit, "GIT_HTTP_LOW_SPEED_LIMIT");
>  	set_long_from_env(&curl_low_speed_time, "GIT_HTTP_LOW_SPEED_TIME");

This, along with other environment variables, is processed after
repo_config() collects configured values by triggering the
http_options() callback, so the environment overrides the configured
value, as expected.

> diff --git a/t/t5585-http-connect-timeout.sh b/t/t5585-http-connect-timeout.sh
> new file mode 100755
> index 0000000000..7363e23bfe
> --- /dev/null
> +++ b/t/t5585-http-connect-timeout.sh
> @@ -0,0 +1,60 @@
> +#!/bin/sh
> +
> +test_description='test http.connecttimeoutms and GIT_HTTP_CONNECT_TIMEOUT_MS'
> +
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> +start_httpd

What are we testing with this new script, really?

As far as I can see, nobody is sitting next to the running test
with a stopwatch to ensure that the client times out as specified.  Should
we really consume a limited shared resource, the four-digit test
number, for this instead of adding a few "not a number (should fail
to parse)" tests to existing http tests?

Thanks.