Re: [PATCH 2/4] http: add the ability to log progress

From: Jeff King <peff@peff.net>
Date: May 9, 2024, 16:52 UTC
Message-ID: <20240509165212.GC1708095@coredump.intra.peff.net>
In-Reply-To: <20240508124453.600871-3-toon@iotcl.com>

On Wed, May 08, 2024 at 02:44:51PM +0200, Toon Claes wrote:

Show 18 quoted lines

> @@ -2017,6 +2021,21 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)
>  #define HTTP_REQUEST_STRBUF	0
>  #define HTTP_REQUEST_FILE	1
>  
> +static int http_progress_callback(void *clientp, curl_off_t dltotal,
> +				  curl_off_t dlnow, curl_off_t ultotal,
> +				  curl_off_t ulnow)
> +{
> +	struct progress *progress = clientp;
> +
> +	if (progress) {
> +		progress_set_total(progress, dltotal);
> +		display_progress(progress, dlnow);
> +		display_throughput(progress, dlnow);
> +	}
> +
> +	return 0;
> +}

A very long time ago I implemented something similar (for a proto-bundle-uri feature), and I found out the hard way that both curl and our progress code may rely on SIGALRM (curl may use it to put a timeout on blocking calls that don't support select, like DNS resolution). Making things even more confusing, it's platform dependent, since on modern platforms it spawns a separate resolving thread to avoid blocking the main one.

So it seems to work OK for me on Linux without anything further, but we may want this to be extra careful:

-- >8 --
Date: Thu, 10 Nov 2011 01:29:55 -0500
Subject: [PATCH] http: turn off curl signals

Curl sets and clears the handler for SIGALRM, which makes it incompatible with git's progress code. However, we can ask curl not to do this.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/http.c b/http.c
index 752c879c1f..e7986e1353 100644
--- a/http.c
+++ b/http.c
@@ -1230,6 +1230,8 @@ static CURL *get_curl_handle(void)
 
 	set_curl_keepalive(result);
 
+	curl_easy_setopt(result, CURLOPT_NOSIGNAL, 1);
+
 	return result;
 }
 
-- 8< --

In the other part of the thread I suggested the possibility of having
remote-https shuttle machine-readable progress back to the caller. That
would also solve this issue, because the actual display_progress() calls
would happen in the parent.

-Peff