| Message ID | cover.1742423021.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | http: support fine-tuning curl's keepalive behavior | expand |
On Wed, Mar 19, 2025 at 3:37 PM Taylor Blau <me@ttaylorr.com> wrote: > > 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 <me@ttaylorr.com> > > @@ 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 <me@ttaylorr.com> > > ## 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 The new patch and range-diff look good to me.
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 <me@ttaylorr.com> @@ 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 <me@ttaylorr.com> ## 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