From: Jeff King <peff@peff.net>
Date: Thu, 05 Mar 2026 00:56:11 GMT
Subject: Re: [PATCH v2 02/10] upload-pack: adapt keepalives based on buffering
Message-ID: <20260305005611.GB4943@coredump.intra.peff.net>
In-Reply-To: <20260303-pks-upload-pack-write-contention-v2-2-7321830f08fe@pks.im>

On Tue, Mar 03, 2026 at 04:00:17PM +0100, Patrick Steinhardt wrote:

> The most important edge case here happens in `relay_pack_data()`. When
> we haven't seen the initial "PACK" signature from git-pack-objects(1)
> yet we buffer incoming data. So in the worst case, if each of the bytes
> of that signature arrive shortly before the configured keepalive
> timeout, then we may not send out any data for a time period that is
> (almost) four times as long as the configured timeout.

Thanks for laying out this case. I think this is all-but-impossible in
practice, as anybody writing "PACK" is going to do so all at once. Even
4 separate write() calls would be fine, as long as it does not pause in
between!

I think there's another one, too. If we are getting packfile_uris, and
pack-objects writes half a line, we will pause waiting for the complete
line to show up. This also seems quite unlikely in practice.

> This edge case is rather unlikely to matter in practice. But in a
> subsequent commit we're going to adapt our buffering mechanism to become
> more aggressive, which makes it more likely that we don't send any data
> for an extended amount of time.
> 
> Adapt the logic so that instead of using a fixed timeout on every call
> to poll(3p), we instead figure out how much time has passed since the
> last-sent data.

OK. That should not be too bad to do, though...

> @@ -365,10 +373,14 @@ static void create_pack_file(struct upload_pack_data *pack_data,
>  	 */
>  
>  	while (1) {
> +		uint64_t now_ms = getnanotime() / 1000000;

...now we are talking about wall-clock time since the epoch. What
happens if time goes backwards due to a clock reset?

Then now_ms may be less than last_sent_ms, and then here:

> +		} else {
> +			/*
> +			 * The polling timeout needs to be adjusted based on
> +			 * the time we have sent our last package. The longer
> +			 * it's been in the past, the shorter the timeout
> +			 * becomes until we eventually don't block at all.
> +			 */
> +			polltimeout_ms = 1000 * pack_data->keepalive - (now_ms - last_sent_ms);
> +			if (polltimeout_ms < 0)
> +				polltimeout_ms = 0;
> +		}

...we end up with a value higher than the keepalive, and we wait too
long. That's probably an OK outcome for such an exceptional condition.
The worst case if you set your clock back is that we fail to send
keepalives until the next actual data chunk arrives.

>  		if (0 <= pe && (pfd[pe].revents & (POLLIN|POLLHUP))) {
>  			/* Status ready; we ship that in the side-band
>  			 * or dump to the standard error.
>  			 */
>  			sz = xread(pack_objects.err, progress,
>  				  sizeof(progress));
> -			if (0 < sz)
> +			if (0 < sz) {
>  				send_client_data(2, progress, sz,
>  						 pack_data->use_sideband);
> -			else if (sz == 0) {
> +				last_sent_ms = now_ms;
> +			} else if (sz == 0) {
>  				close(pack_objects.err);
>  				pack_objects.err = -1;
>  			}

OK, so here we know we sent data, because we pass along stderr as
quickly as possible. If we got EOF on stderr, that doesn't count as
sending data, because we don't pass along that EOF to the client. Makes
sense.

>  		if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
> +			bool did_send_data;
>  			int result = relay_pack_data(pack_objects.out,
>  						     output_state,
>  						     pack_data->use_sideband,
> -						     !!uri_protocols);
> +						     !!uri_protocols,
> +						     &did_send_data);

And then for stdout, we have to ask relay_pack_data() if it sent
anything. That makes sense.

-Peff