Re: [PATCH] add http.maxReceiveSpeed to limit git-receive-pack receiving speed

Xia XiaoWen [off-list ref] writes:

Sometimes need to limit the receive speed of git `clone/fetch`
because of the limited network bandwidth, otherwise will prevent
other applications from using the network normally.

No subject in these two half-sentences.

    In order to avoid hogging all the available bandwidth, users may
    want to limit the speed to receive traffic for "git clone" or
    "git fetch".

perhaps.

Add `http.maxReceiveSpeed` to limit `git-receive-pack` receiving

"limit `git-receive-pack`'s" or "limit receiving speedk of ..."

speed, Can be overridden by `GIT_HTTP_MAX_RECEIVE_SPEED` eivironment
variable.

The default is unlimited, same if the value is 0 or negative. The

Let's error it out when the configuration gives a value that does
not make sense instead.  That way, we could in the future use some
of these invalid values to signal special behaviour if we wanted to.

default unit is Bytes/s, common unit suffixes of k, m, or g are supported.

OK.

this configuration is valid for `clone`, `fetch`, `pull` commands of the

s/this/This/

https protocol, and only supports libcurl 7.15.5 and above.

We are likely be raising the floor versions of libcURL to 7.16.0 or
even 7.19.4 soonish.  It probably would make it easier to allow it
unconditionally (otherwise you'd probably need to implement error or
warning messages when configuration is given but the libcURL version
used is too old, etc.).

On cURL side, CURLOPT_MAX_RECV_SPEED_LARGE takes curl_off_t, which
is typically off_t (which is signed).

I wonder if ssize_t is overkill for our purpose, though.  Can't this
be a plain vanilla "int" or perhaps "long", just like the variable
defined above uses "long" for "speed"?  Or is 2gb/s too low to be
practical and we must use a 64-bit type?

Check for nonsense values, so that we can later use them to mean
something special.  It is good to remember is that you can always
loosen the rules after you give your software to your users, but it
is very hard to tighten the rules.  As you never need more than one
way to specify "the default" (aka "unlimited"), reserving any
non-positive value to mean the default is a design that is
extensible poorly.

I.e. insert something like

		if (curl_max_receive_speed < 0)
			die("negatigve number for %s: %s", var, value);

here.

The "result" handle was created anew in this function, so the
distinction does not really matter in practrice, but since you are
carefully initializing the variable to "-1" so that we can
differentiate the case where it is unconfigured (hence we want to
use the default) and it is set to zero (hence we want to use the
default), it would be more consistent and future-proof if you also
allowed 0 to be passed here, i.e.

	if (curl_max_receive_speed >= 0)

+		curl_easy_setopt(result, CURLOPT_MAX_RECV_SPEED_LARGE,
+				 curl_max_receive_speed);

This call, if you use anything but curl_off_t as the type for
curl_max_receive_speed variable where it is declared, needs a cast,
like in the example https://curl.se/libcurl/c/CURLOPT_MAX_RECV_SPEED_LARGE.html

Overlong line.

Unlike curl_max_receive_speed that must be visible and
understandable long haul in this file, the temporary string variable
lives only during these handful of lines and shortened name is
easier to see and understand what is going on.  Also, you can avoid
repeated spelling out of the environment variable name by giving a
constant for it near the top of this function, e.g.

	static const char mrs_env[] = "GIT_HTTP_MAX_RECEIVE_SPEED";

Then this part would become:

	if (mrs) {
		if (!git_parse_ssize_t(mrs, &curl_max_receive_speed))
			die(_("invalid number for %s: %s", mrs_env, mrs);
		if (curl_max_receive_speed < 0)
			die(_("negative number for %s: %s", mrs_env, mrs);
	}

+		warning("failed to parse GIT_HTTP_MAX_RECEIVE_SPEED: %s", max_receive_speed);

Thanks.

On Thu, Aug 19, 2021 at 11:28:53AM -0700, Junio C Hamano wrote:

quoted

https protocol, and only supports libcurl 7.15.5 and above.

We are likely be raising the floor versions of libcURL to 7.16.0 or
even 7.19.4 soonish.  It probably would make it easier to allow it
unconditionally (otherwise you'd probably need to implement error or
warning messages when configuration is given but the libcURL version
used is too old, etc.).

Yeah, I agree that if we can drop the conditional totally, we should.

If we do need to make it conditional, I think there was a preference for
shifting to checking the actual option constants, like:

  #ifdef CURLOPT_MAX_RECV_SPEED_LARGE
  ...ok, we have it...
  #endif

The rest of your comments all seemed quite reasonable to me.

As a general feature, IMHO speed-limiting is best done outside of
application-level tools like Git (and instead done via proxies or kernel
network config). But since we are not building the feature ourselves,
but rather just plugging our config to curl's feature, I don't have any
problem with it here.

-Peff

On August 20, 2021 2:12 PM, Jeff King wrote:

On Thu, Aug 19, 2021 at 11:28:53AM -0700, Junio C Hamano wrote:

quoted

quoted

https protocol, and only supports libcurl 7.15.5 and above.

We are likely be raising the floor versions of libcURL to 7.16.0 or
even 7.19.4 soonish.  It probably would make it easier to allow it
unconditionally (otherwise you'd probably need to implement error or
warning messages when configuration is given but the libcURL version
used is too old, etc.).

Yeah, I agree that if we can drop the conditional totally, we should.

If we do need to make it conditional, I think there was a preference for shifting to checking the actual option constants, like:

 #ifdef CURLOPT_MAX_RECV_SPEED_LARGE
 ...ok, we have it...
 #endif

The rest of your comments all seemed quite reasonable to me.

As a general feature, IMHO speed-limiting is best done outside of application-level tools like Git (and instead done via proxies or kernel
network config). But since we are not building the feature ourselves, but rather just plugging our config to curl's feature, I don't have any
problem with it here.

I second that. This should be in the realm of QoS at a managed switch/router in the consumer's network (or the upstream repo in some cases).
-Randall

Hi Junio:

Thank You so much for your reply!!

quoted

The default is unlimited, same if the value is 0 or negative.

Let's error it out when the configuration gives a value that does
not make sense instead. That way, we could in the future use some
of these invalid values to signal special behaviour if we wanted to.

But this patch is similar to the `http.lowspeedlimit` and `http.lowspeedtime`.
And `http.lowspeedlimit` will not error out the negative values:

        if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
            curl_easy_setopt(result, CURLOPT_LOW_SPEED_LIMIT,
                 curl_low_speed_limit);
            curl_easy_setopt(result, CURLOPT_LOW_SPEED_TIME,
                 curl_low_speed_time);
        }

We are likely be raising the floor versions of libcURL to 7.16.0 or
even 7.19.4 soonish.

OK, I will remove the #if #endif block for libcurl 7.15.5.

Let's error it out when the configuration gives a value that does
not make sense instead.

The same as above.

I wonder if ssize_t is overkill for our purpose, though.

You are right, I will change it to long type.

This call, if you use anything but curl_off_t as the type for
curl_max_receive_speed variable where it is declared, needs a cast,

You are right.

Unlike curl_max_receive_speed that must be visible and
understandable long haul in this file, the temporary string variable
lives only during these handful of lines and shortened name is
easier to see and understand what is going on.

OK.


Junio C Hamano [off-list ref] 于2021年8月20日周五 上午2:28写道：

Xia XiaoWen [off-list ref] writes:

quoted

Sometimes need to limit the receive speed of git `clone/fetch`
because of the limited network bandwidth, otherwise will prevent
other applications from using the network normally.

No subject in these two half-sentences.

    In order to avoid hogging all the available bandwidth, users may
    want to limit the speed to receive traffic for "git clone" or
    "git fetch".

perhaps.

quoted

Add `http.maxReceiveSpeed` to limit `git-receive-pack` receiving

"limit `git-receive-pack`'s" or "limit receiving speedk of ..."

quoted

speed, Can be overridden by `GIT_HTTP_MAX_RECEIVE_SPEED` eivironment
variable.

The default is unlimited, same if the value is 0 or negative. The

Let's error it out when the configuration gives a value that does
not make sense instead.  That way, we could in the future use some
of these invalid values to signal special behaviour if we wanted to.

quoted

default unit is Bytes/s, common unit suffixes of k, m, or g are supported.

OK.

quoted

this configuration is valid for `clone`, `fetch`, `pull` commands of the

s/this/This/

quoted

https protocol, and only supports libcurl 7.15.5 and above.

We are likely be raising the floor versions of libcURL to 7.16.0 or
even 7.19.4 soonish.  It probably would make it easier to allow it
unconditionally (otherwise you'd probably need to implement error or
warning messages when configuration is given but the libcURL version
used is too old, etc.).

quoted

---
 http.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 8119247149..12030cf3bc 100644
--- a/http.c
+++ b/http.c

@@ -83,6 +83,9 @@ static const char *ssl_pinnedkey;
 static const char *ssl_cainfo;
 static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
+#if LIBCURL_VERSION_NUM >= 0x070f05
+static ssize_t curl_max_receive_speed = -1;

On cURL side, CURLOPT_MAX_RECV_SPEED_LARGE takes curl_off_t, which
is typically off_t (which is signed).

I wonder if ssize_t is overkill for our purpose, though.  Can't this
be a plain vanilla "int" or perhaps "long", just like the variable
defined above uses "long" for "speed"?  Or is 2gb/s too low to be
practical and we must use a 64-bit type?

quoted

+#endif
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static const char *http_proxy_authmethod;

@@ -361,7 +364,12 @@ static int http_options(const char *var, const char *value, void *cb)
              curl_low_speed_time = (long)git_config_int(var, value);
              return 0;
      }
-
+#if LIBCURL_VERSION_NUM >= 0x070f05
+     if (!strcmp("http.maxreceivespeed", var)) {
+             curl_max_receive_speed = git_config_ssize_t(var, value);

Check for nonsense values, so that we can later use them to mean
something special.  It is good to remember is that you can always
loosen the rules after you give your software to your users, but it
is very hard to tighten the rules.  As you never need more than one
way to specify "the default" (aka "unlimited"), reserving any
non-positive value to mean the default is a design that is
extensible poorly.

I.e. insert something like

                if (curl_max_receive_speed < 0)
                        die("negatigve number for %s: %s", var, value);

here.

quoted

+             return 0;
+     }
+#endif
      if (!strcmp("http.noepsv", var)) {
              curl_ftp_no_epsv = git_config_bool(var, value);
              return 0;

@@ -974,6 +982,12 @@ static CURL *get_curl_handle(void)
                               curl_low_speed_time);
      }

+#if LIBCURL_VERSION_NUM >= 0x070f05
+     if (curl_max_receive_speed > 0)

The "result" handle was created anew in this function, so the
distinction does not really matter in practrice, but since you are
carefully initializing the variable to "-1" so that we can
differentiate the case where it is unconfigured (hence we want to
use the default) and it is set to zero (hence we want to use the
default), it would be more consistent and future-proof if you also
allowed 0 to be passed here, i.e.

        if (curl_max_receive_speed >= 0)

quoted

+             curl_easy_setopt(result, CURLOPT_MAX_RECV_SPEED_LARGE,
+                              curl_max_receive_speed);

This call, if you use anything but curl_off_t as the type for
curl_max_receive_speed variable where it is declared, needs a cast,
like in the example https://curl.se/libcurl/c/CURLOPT_MAX_RECV_SPEED_LARGE.html

quoted

+#endif
+
      curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
 #if LIBCURL_VERSION_NUM >= 0x071301
      curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);

@@ -1105,6 +1119,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 {
      char *low_speed_limit;
      char *low_speed_time;
+#if LIBCURL_VERSION_NUM >= 0x070f05
+     char *max_receive_speed;
+#endif
      char *normalized_url;
      struct urlmatch_config config = { STRING_LIST_INIT_DUP };

@@ -1196,6 +1213,11 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
      low_speed_time = getenv("GIT_HTTP_LOW_SPEED_TIME");
      if (low_speed_time != NULL)
              curl_low_speed_time = strtol(low_speed_time, NULL, 10);
+#if LIBCURL_VERSION_NUM >= 0x070f05
+     max_receive_speed = getenv("GIT_HTTP_MAX_RECEIVE_SPEED");
+     if (max_receive_speed && !git_parse_ssize_t(max_receive_speed, &curl_max_receive_speed))

Overlong line.

Unlike curl_max_receive_speed that must be visible and
understandable long haul in this file, the temporary string variable
lives only during these handful of lines and shortened name is
easier to see and understand what is going on.  Also, you can avoid
repeated spelling out of the environment variable name by giving a
constant for it near the top of this function, e.g.

        static const char mrs_env[] = "GIT_HTTP_MAX_RECEIVE_SPEED";

Then this part would become:

        if (mrs) {
                if (!git_parse_ssize_t(mrs, &curl_max_receive_speed))
                        die(_("invalid number for %s: %s", mrs_env, mrs);
                if (curl_max_receive_speed < 0)
                        die(_("negative number for %s: %s", mrs_env, mrs);
        }

quoted

+             warning("failed to parse GIT_HTTP_MAX_RECEIVE_SPEED: %s", max_receive_speed);

Thanks.