| Message ID | d84041580895a653648ee2370e21d7d2aa4fc4bb.1742336481.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | http: support fine-tuning curl's keepalive behavior | expand |
On Tue, Mar 18, 2025 at 06:21:41PM -0400, Taylor Blau wrote: > 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. 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. > diff --git a/http.c b/http.c > index 526f9680f9..c13c7da530 100644 > --- a/http.c > +++ b/http.c > @@ -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; > + } > + > /* 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`. Patrick
On Wed, Mar 19, 2025 at 9:00 AM Patrick Steinhardt <ps@pks.im> wrote: > > On Tue, Mar 18, 2025 at 06:21:41PM -0400, Taylor Blau wrote: > > 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. > > 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. Was going to comment with the same question.
On Wed, Mar 19, 2025 at 05:00:51PM +0100, Patrick Steinhardt wrote: > On Tue, Mar 18, 2025 at 06:21:41PM -0400, Taylor Blau wrote: > > 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. > > 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. No; the defaults are sufficient for most users. It's the users that have extremely slow connections and/or are talking to hosts that have very short timeouts for inactive TCP connections that would want to change these values. > > diff --git a/http.c b/http.c > > index 526f9680f9..c13c7da530 100644 > > --- a/http.c > > +++ b/http.c > > @@ -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; > > + } > > + > > /* 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`. No, they're not. They match the style of the rest of this file, but we shouldn't have explicit casts anywhere. I'll send a new round with an additional patch that drops the unnecessary casts, too. Thanks, Taylor
diff --git a/Documentation/config/http.adoc b/Documentation/config/http.adoc index 22a8803dea..67393282fa 100644 --- a/Documentation/config/http.adoc +++ b/Documentation/config/http.adoc @@ -296,6 +296,24 @@ http.lowSpeedLimit, http.lowSpeedTime:: Can be overridden by the `GIT_HTTP_LOW_SPEED_LIMIT` and `GIT_HTTP_LOW_SPEED_TIME` environment variables. +http.keepAliveIdle:: + Specifies how long in seconds to wait on an idle connection + before sending TCP keepalive probes (if supported by the OS). If + unset, curl's default value is used. Can be overridden by the + `GIT_HTTP_KEEPALIVE_IDLE` environment variable. + +http.keepAliveInterval:: + Specifies how long in seconds to wait between TCP keepalive + probes (if supported by the OS). If unset, curl's default value + is used. Can be overridden by the `GIT_HTTP_KEEPALIVE_INTERVAL` + environment variable. + +http.keepAliveCount:: + Specifies how many TCP keepalive probes to send before giving up + and terminating the connection (if supported by the OS). If + unset, curl's default value is used. Can be overridden by the + `GIT_HTTP_KEEPALIVE_COUNT` environment variable. + http.noEPSV:: A boolean which disables using of EPSV ftp command by curl. This can be helpful with some "poor" ftp servers which don't diff --git a/http.c b/http.c index 526f9680f9..c13c7da530 100644 --- a/http.c +++ b/http.c @@ -104,6 +104,10 @@ static struct { }; #endif +static long curl_tcp_keepidle = -1; +static long curl_tcp_keepintvl = -1; +static long curl_tcp_keepcnt = -1; + enum proactive_auth { PROACTIVE_AUTH_NONE = 0, PROACTIVE_AUTH_IF_CREDENTIALS, @@ -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; + } + /* Fall back on the default ones */ return git_default_config(var, value, ctx, data); } @@ -704,7 +721,6 @@ static int has_proxy_cert_password(void) return 1; } - /* Return 1 if redactions have been made, 0 otherwise. */ static int redact_sensitive_header(struct strbuf *header, size_t offset) { @@ -1240,6 +1256,15 @@ static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_TCP_KEEPALIVE, 1); + if (curl_tcp_keepidle > -1) + curl_easy_setopt(result, CURLOPT_TCP_KEEPIDLE, + curl_tcp_keepidle); + if (curl_tcp_keepintvl > -1) + curl_easy_setopt(result, CURLOPT_TCP_KEEPINTVL, + curl_tcp_keepintvl); + if (curl_tcp_keepcnt > -1) + curl_easy_setopt(result, CURLOPT_TCP_KEEPCNT, curl_tcp_keepcnt); + return result; } @@ -1367,6 +1392,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth) ssl_cert_password_required = 1; } + set_long_from_env(&curl_tcp_keepidle, "GIT_TCP_KEEPIDLE"); + set_long_from_env(&curl_tcp_keepintvl, "GIT_TCP_KEEPINTVL"); + set_long_from_env(&curl_tcp_keepcnt, "GIT_TCP_KEEPCNT"); + curl_default = get_curl_handle(); }
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. 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. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/config/http.adoc | 18 ++++++++++++++++++ http.c | 31 ++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 1 deletion(-)