Skip to contentSkip to conversation

http: add a config to limit the connection time

PATCH In review GalaxySnail via GitGitGadget 2 participants 2 messages Jul 23

Subsystems (2)
Documentation Arm/Amlogic Meson SoC support
lore
Share & tools
lore

GalaxySnail via GitGitGadget

author
#0 lore raw

PATCH 1/1 http: add a config to limit the connection time

From: GalaxySnail <[email protected]>

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 <[email protected]>

---
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
Documentation/config/http.adoc +7 -0
index 792a71b413..a4f7afa61e 100644
@@ -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
http.c +11 -0
index caccf2108e..befe9ea8a0 100644
@@ -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");
t/meson.build +1 -0
index 8ae6ab6c5f..6196736cb2 100644
@@ -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',
t/t5585-http-connect-timeout.sh new file +60 -0

60 lines not on this page · raw message

base-commit: 9a0c4701dcd5725c4184599322b52933ff5005ca

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 4 lines · GalaxySnail via GitGitGadget

From: GalaxySnail <[email protected]>

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?

Quoted 5 lines

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 <[email protected]>

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

Quoted 12 lines
 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
Documentation/config/http.adoc +0 -0
index 792a71b413..a4f7afa61e 100644

Documentation/config/http.adoc for most push problems, but can increase memory consumption · line 308

Quoted 9 lines
Documentation/config/http.adoc +6 -0
@@ -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.

Quoted 13 lines
http.c +1 -0
index caccf2108e..befe9ea8a0 100644
@@ -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;

http.c static int http_options(const char *var, const char *value, · line 457

Quoted 7 lines
http.c +4 -0
@@ -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.

http.c static CURL *get_curl_handle(void) · line 1225

Quoted 7 lines
http.c +3 -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);

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.

http.c void http_init(struct remote *remote, const char *url, int proactive_auth) · line 1489

Quoted 8 lines
http.c +2 -0
@@ -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.

t/t5585-http-connect-timeout.sh line 7

Quoted 13 lines
t/t5585-http-connect-timeout.sh new file +7 -0
new file mode 100755
index 0000000000..7363e23bfe
@@ -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.