Re: [PATCH 3/3] http.c: allow custom TCP keepalive behavior via config

[Taylor Blau <me@ttaylorr.com>]

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