http: support fine-tuning curl's keepalive behavior

In 7059cd99fc (http_init(): Fix config file parsing, 2009-03-09), http.c gained a new "set_from_env()" function as a convenience function around conditionally assigning an environment variable to some variable if and only if the environment variable was set to begin with.

But prior to 7059cd99fc, there were two spots which need to first strtol() whatever is set in the environment before assigning it to a long pointer. Both instances stored the result of getenv() in a temporary variable, and conditionally strtol() it depending on whether or not getenv() returned NULL.

Replace those two instances with a new cousin of 'set_from_env()' called 'set_long_from_env()', which does what its name suggests. This allows us to remove the temporary variables and clean up some minor code duplication. More importantly, however, it prepares us for a future commit which will introduce more instances of assigning an environment variable to a long.

Signed-off-by Taylor Blau <[email protected]>

---
 http.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

http.c +9 -9

index 0c9a872809..be564fd520 100644
@@ -1256,10 +1256,15 @@ static void set_from_env(char **var, const char *envname)
        }
 }
 
+static void set_long_from_env(long *var, const char *envname)
+{
+       const char *val = getenv(envname);
+       if (val)
+               *var = strtol(val, NULL, 10);
+}
+
 void http_init(struct remote *remote, const char *url, int proactive_auth)
 {
-       char *low_speed_limit;
-       char *low_speed_time;
        char *normalized_url;
        struct urlmatch_config config = URLMATCH_CONFIG_INIT;
 

@@ -1338,12 +1343,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 
        set_from_env(&user_agent, "GIT_HTTP_USER_AGENT");
 
-       low_speed_limit = getenv("GIT_HTTP_LOW_SPEED_LIMIT");
-       if (low_speed_limit)
-               curl_low_speed_limit = strtol(low_speed_limit, NULL, 10);
-       low_speed_time = getenv("GIT_HTTP_LOW_SPEED_TIME");
-       if (low_speed_time)
-               curl_low_speed_time = strtol(low_speed_time, NULL, 10);
+       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");
 
        if (curl_ssl_verify == -1)
                curl_ssl_verify = 1;
--

2.49.0.3.gbb7a4a684c.dirty

In 7059cd99fc (http_init(): Fix config file parsing, 2009-03-09), http.c gained a new "set_from_env()" function as a convenience function around conditionally assigning an environment variable to some variable if and only if the environment variable was set to begin with.

But prior to 7059cd99fc, there were two spots which need to first strtol() whatever is set in the environment before assigning it to a long pointer. Both instances stored the result of getenv() in a temporary variable, and conditionally strtol() it depending on whether or not getenv() returned NULL.

Replace those two instances with a new cousin of 'set_from_env()' called 'set_long_from_env()', which does what its name suggests. This allows us to remove the temporary variables and clean up some minor code duplication. More importantly, however, it prepares us for a future commit which will introduce more instances of assigning an environment variable to a long.

Signed-off-by Taylor Blau <[email protected]>

---
 http.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

http.c +0 -0

index 0c9a872809..be564fd520 100644

curl supports a few options to control when and how often it should instruct the OS to send TCP keepalives, like KEEPIDLE, KEEPINTVL, and KEEPCNT. Until this point, there hasn't been a way for users to change what values are used for these options, forcing them to rely on curl's defaults.

But we do unconditionally enable TCP keepalives without giving users an ability to tweak any fine-grained parameters. Ordinarily this isn't a problem, particularly for users that have fast-enough connections, and/or are talking to a server that has generous or nonexistent thresholds for killing a connection it hasn't heard from in a while.

But it can present a problem when one or both of those assumptions fail. For instance, I can reliably get an in-progress clone to be killed from the remote end when cloning from some forges while using trickle to limit my clone's bandwidth.

http.c +6 -0

index 526f9680f9..c13c7da530 100644
@@ -557,6 +561,19 @@ static int http_options(const char *var, const char *value,
 		return 0;
 	}
 
+	if (!strcmp("http.keepaliveidle", var)) {
+		curl_tcp_keepidle = (long)git_config_int(var, value, ctx->kvi);
+		return 0;
+	}
+	if (!strcmp("http.keepaliveinterval", var)) {
+		curl_tcp_keepintvl = (long)git_config_int(var, value, ctx->kvi);

At the end of get_curl_handle() we call set_curl_keepalive() to enable TCP keepalive probes on our CURL handle. set_curl_keepalive() dates back to 47ce115370 (http: use curl's tcp keepalive if available, 2013-10-14), which conditionally compiled different variants of set_curl_keepalive() depending on what version of curl we were compiled with[^1].

As of f7c094060c (git-curl-compat: remove check for curl 7.25.0, 2024-10-23), we no longer conditionally compile set_curl_keepalive() since we no longer support pre-7.25.0 versions of curl. But the version of that function that we kept is really just a thin wrapper around setting the TCP_KEEPALIVE option, so there's no reason to keep it in its own function.

Inline the definition of set_curl_keepalive() to within get_curl_handle() so that the setup of our CURL handle is self-contained.

[1]: The details are spelled out in 47ce115370, but the gist is curl
7.25.0 and newer use CURLOPT_TCP_KEEPALIVE, older versions use CURLOPT_SOCKOPTFUNCTION with a custom callback, and older versions that predate even that option do nothing.

Signed-off-by Taylor Blau <[email protected]>

---
 http.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

http.c +1 -6

index be564fd520..526f9680f9 100644
@@ -704,10 +704,6 @@ static int has_proxy_cert_password(void)
        return 1;
 }
 
-static void set_curl_keepalive(CURL *c)
-{
-       curl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1);
-}
 
 /* Return 1 if redactions have been made, 0 otherwise. */
 static int redact_sensitive_header(struct strbuf *header, size_t offset)

@@ -1242,7 +1238,7 @@ static CURL *get_curl_handle(void)
        }
        init_curl_proxy_auth(result);
 
-       set_curl_keepalive(result);
+       curl_easy_setopt(result, CURLOPT_TCP_KEEPALIVE, 1);
 
        return result;
 }
--

2.49.0.3.gbb7a4a684c.dirty

Quoted 12 lines · Taylor Blau

curl supports a few options to control when and how often it should instruct the OS to send TCP keepalives, like KEEPIDLE, KEEPINTVL, and KEEPCNT. Until this point, there hasn't been a way for users to change what values are used for these options, forcing them to rely on curl's defaults.

But we do unconditionally enable TCP keepalives without giving users an ability to tweak any fine-grained parameters. Ordinarily this isn't a problem, particularly for users that have fast-enough connections, and/or are talking to a server that has generous or nonexistent thresholds for killing a connection it hasn't heard from in a while.

Quoted 4 lines

But it can present a problem when one or both of those assumptions fail. For instance, I can reliably get an in-progress clone to be killed from the remote end when cloning from some forges while using trickle to limit my clone's bandwidth.

Does this mean that our defaults are insufficient, as well? It's nice to add a way to adapt those settings for the future, but ideally no user should ever have to manually tweak them and Git should work out of the box.

Quoted 8 lines

http.c +0 -0

index 0c9a872809..be564fd520 100644
@@ -1256,10 +1256,15 @@ static void set_from_env(char **var, const char *envname)
 	}
 }
 

http.c static void set_from_env(char **var, const char *envname) · line 1264

Quoted 6 lines

http.c +6 -0

+static void set_long_from_env(long *var, const char *envname)
+{
+	const char *val = getenv(envname);
+	if (val)
+		*var = strtol(val, NULL, 10);
+}

Hm. We don't perform any error checking at all for whether or not the value of the environment variable is a valid integer. This isn't a new issue introduced by your patch, but now that we have a central place where it's being parsed I wonder whether we should be checking for errors?

Quoted 12 lines · Taylor Blau

curl supports a few options to control when and how often it should instruct the OS to send TCP keepalives, like KEEPIDLE, KEEPINTVL, and KEEPCNT. Until this point, there hasn't been a way for users to change what values are used for these options, forcing them to rely on curl's defaults.

But we do unconditionally enable TCP keepalives without giving users an ability to tweak any fine-grained parameters. Ordinarily this isn't a problem, particularly for users that have fast-enough connections, and/or are talking to a server that has generous or nonexistent thresholds for killing a connection it hasn't heard from in a while.

Quoted 4 lines

But it can present a problem when one or both of those assumptions fail. For instance, I can reliably get an in-progress clone to be killed from the remote end when cloning from some forges while using trickle to limit my clone's bandwidth.

Does this mean that our defaults are insufficient, as well? It's nice to add a way to adapt those settings for the future, but ideally no user should ever have to manually tweak them and Git should work out of the box.

Quoted 19 lines

http.c +11 -0

index 526f9680f9..c13c7da530 100644
@@ -557,6 +561,19 @@ static int http_options(const char *var, const char *value,
 		return 0;
 	}
 
+	if (!strcmp("http.keepaliveidle", var)) {
+		curl_tcp_keepidle = (long)git_config_int(var, value, ctx->kvi);
+		return 0;
+	}
+	if (!strcmp("http.keepaliveinterval", var)) {
+		curl_tcp_keepintvl = (long)git_config_int(var, value, ctx->kvi);
+		return 0;
+	}
+	if (!strcmp("http.keepalivecount", var)) {
+		curl_tcp_keepcnt = (long)git_config_int(var, value, ctx->kvi);
+		return 0;

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

Quoted 5 lines

http.c +2 -0

+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, ctx, data);
 }

Are the casts really necessary? The compiler shouldn't complain when promoting from int to long.

Here's a reroll of my series to introduce new http.* knobs to control curl's TCP keepalive behavior.

This reroll is mostly minor, with the notable differences being limited to:

- Added error handling in the new http.c::set_long_from_env().

- Removed unnecessary casts from int -> long.

- Only set CURLOPT_TCP_KEEPCNT when compiled with a version of curl that knows about that option to begin with (>8.9.0).

As usual, a range-diff is included below for convenience. Thanks again for reviewing!

== Original cover letter

This short series introduces a few new http.* configuration options to control curl's behavior around TCP keepalive packets. The details are spelled out in the final patch, but the gist is:

- http.keepAliveIdle specifies how long in seconds to wait on an idle connection before beginning to send keepalive packets.

- http.keepAliveInterval does the same but controls the interval between successive keepalive packets.

- http.keepAliveCount specifies how many keepalive packets to send before closing down the connection.

The first two commits of the series are general code clean-up of a couple of small things I noticed while reading through the http.c code, and the final patch implements these new options.

I couldn't think of a great way to test these new configuration settings, and given the simplicity of the final patch I opted for no tests there. But if someone has a good idea of how to test this behavior, please let me know.

In either case, thanks in advance for your review!

Taylor Blau (4):
http.c: remove unnecessary casts to long http.c: introduce set_long_from_env() for convenience http.c: inline set_curl_keepalive()
http.c: allow custom TCP keepalive behavior via config

 Documentation/config/http.adoc | 18 ++++++++
 git-curl-compat.h              |  7 ++++
 http.c                         | 75 ++++++++++++++++++++++++++--------
 3 files changed, 84 insertions(+), 16 deletions(-)

Range-diff against v1:

-:  ---------- > 1:  204e5e18d2 http.c: remove unnecessary casts to long
1:  ba22a121fa ! 2:  2e39a78e87 http.c: introduce `set_long_from_env()` for convenience
    @@ Commit message
         Replace those two instances with a new cousin of 'set_from_env()' called
         'set_long_from_env()', which does what its name suggests. This allows us
         to remove the temporary variables and clean up some minor code
    -    duplication. More importantly, however, it prepares us for a future
    -    commit which will introduce more instances of assigning an environment
    -    variable to a long.
    +    duplication while also adding more robust error handling.
    +
    +    More importantly, however, it prepares us for a future commit which will
    +    introduce more instances of assigning an environment variable to a long.

         Signed-off-by: Taylor Blau <[email protected]>

    @@ http.c: static void set_from_env(char **var, const char *envname)
     +static void set_long_from_env(long *var, const char *envname)
     +{
     +  const char *val = getenv(envname);
    -+  if (val)
    -+          *var = strtol(val, NULL, 10);
    ++  if (val) {
    ++          long tmp;
    ++          char *endp;
    ++          int saved_errno = errno;
    ++
    ++          errno = 0;
    ++          tmp = strtol(val, &endp, 10);
    ++
    ++          if (errno)
    ++                  warning_errno(_("failed to parse %s"), envname);
    ++          else if (*endp || endp == val)
    ++                  warning(_("failed to parse %s"), envname);
    ++          else
    ++                  *var = tmp;
    ++
    ++          errno = saved_errno;
    ++  }
     +}
     +
      void http_init(struct remote *remote, const char *url, int proactive_auth)
2:  a05269552f = 3:  cdfc9baa8d http.c: inline `set_curl_keepalive()`
3:  d840415808 ! 4:  3fe62181e5 http.c: allow custom TCP keepalive behavior via config

@@ Commit message
keepalive behavior, expose configuration and environment variables which allow setting curl's KEEPIDLE, KEEPINTVL, and KEEPCNT options.

+ Note that while KEEPIDLE and KEEPINTVL were added in curl 7.25.0,
+ KEEPCNT was added much more recently in curl 8.9.0. Per f7c094060c
+ (git-curl-compat: remove check for curl 7.25.0, 2024-10-23), both
+ KEEPIDLE and KEEPINTVL are set unconditionally. But since we may be
+ compiled with a curl that isn't as new as 8.9.0, only set KEEPCNT when
+ we have CURLOPT_TCP_KEEPCNT to begin with.
+

Signed-off-by Taylor Blau <[email protected]>

## Documentation/config/http.adoc ##
@@ Documentation/config/http.adoc: http.lowSpeedLimit, http.lowSpeedTime::
A boolean which disables using of EPSV ftp command by curl. This can be helpful with some "poor" ftp servers which don't

+ ## git-curl-compat.h ##
+@@
+ #define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1
+ #endif
+
++/**
++ * CURLOPT_TCP_KEEPCNT was added in 8.9.0, released in July, 2024. ++ */
++#if LIBCURL_VERSION_NUM >= 0x080900
++#define GIT_CURL_HAVE_CURLOPT_TCP_KEEPCNT
++#endif
++
+ #endif
+
## http.c ##
@@ http.c: static struct {
};
@@ http.c: static int http_options(const char *var, const char *value,
}

+ if (!strcmp("http.keepaliveidle", var)) {
-+ curl_tcp_keepidle = (long)git_config_int(var, value, ctx->kvi);
++ curl_tcp_keepidle = git_config_int(var, value, ctx->kvi);
+ return 0;
+ }
+ if (!strcmp("http.keepaliveinterval", var)) {
-+ curl_tcp_keepintvl = (long)git_config_int(var, value, ctx->kvi);
++ curl_tcp_keepintvl = git_config_int(var, value, ctx->kvi);
+ return 0;
+ }
+ if (!strcmp("http.keepalivecount", var)) {
-+ curl_tcp_keepcnt = (long)git_config_int(var, value, ctx->kvi);
++ curl_tcp_keepcnt = git_config_int(var, value, ctx->kvi);
+ return 0;
+ }
+
@@ http.c: static CURL *get_curl_handle(void)
+ if (curl_tcp_keepintvl > -1)
+ curl_easy_setopt(result, CURLOPT_TCP_KEEPINTVL,
+ curl_tcp_keepintvl);
++#ifdef GIT_CURL_HAVE_CURLOPT_TCP_KEEPCNT
+ if (curl_tcp_keepcnt > -1)
+ curl_easy_setopt(result, CURLOPT_TCP_KEEPCNT, curl_tcp_keepcnt);
++#endif
+
return result;
}

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e --
2.49.0.4.ge59cf92f8d

http.c +11 -0

index 0cbcb079b2..17b676a1d5 100644
@@ -1256,10 +1256,30 @@ static void set_from_env(char **var, const char *envname)
 	}
 }
 
+static void set_long_from_env(long *var, const char *envname)
+{
+	const char *val = getenv(envname);
+	if (val) {
+		long tmp;
+		char *endp;
+		int saved_errno = errno;
+
+		errno = 0;
+		tmp = strtol(val, &endp, 10);
+

(quoted diff) +11 -0

+static void set_long_from_env(long *var, const char *envname)
+{
+	const char *val = getenv(envname);
+	if (val) {
+		long tmp;
+		char *endp;
+		int saved_errno = errno;
+
+		errno = 0;
+		tmp = strtol(val, &endp, 10);
+

At the end of get_curl_handle() we call set_curl_keepalive() to enable TCP keepalive probes on our CURL handle. set_curl_keepalive() dates back to 47ce115370 (http: use curl's tcp keepalive if available, 2013-10-14), which conditionally compiled different variants of set_curl_keepalive() depending on what version of curl we were compiled with[^1].

As of f7c094060c (git-curl-compat: remove check for curl 7.25.0, 2024-10-23), we no longer conditionally compile set_curl_keepalive() since we no longer support pre-7.25.0 versions of curl. But the version of that function that we kept is really just a thin wrapper around setting the TCP_KEEPALIVE option, so there's no reason to keep it in its own function.

Inline the definition of set_curl_keepalive() to within get_curl_handle() so that the setup of our CURL handle is self-contained.

[1]: The details are spelled out in 47ce115370, but the gist is curl
7.25.0 and newer use CURLOPT_TCP_KEEPALIVE, older versions use CURLOPT_SOCKOPTFUNCTION with a custom callback, and older versions that predate even that option do nothing.

For those users and others who wish to more finely tune the OS's keepalive behavior, expose configuration and environment variables which allow setting curl's KEEPIDLE, KEEPINTVL, and KEEPCNT options.