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

[Junio C Hamano <gitster@pobox.com>]

"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.