From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 10 Mar 2026 12:08:58 GMT
Subject: Re: [PATCH v2 02/10] upload-pack: adapt keepalives based on buffering
Message-ID: <abAJ2iMVa9rwGgr5@pks.im>
In-Reply-To: <20260305005611.GB4943@coredump.intra.peff.net>

On Wed, Mar 04, 2026 at 07:56:11PM -0500, Jeff King wrote:
> 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.

It could happen in case the data was written by the pack-objects hook.
But I fully agree that this is a rather unlikely scenario.

> > 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.

Right. It's rather unlikely to happen, and in addition to that we use a
monotonic clock in `getnanotime()` on systems with `CLOCK_MONOTONIC` and
on Windows. Which probably covers almost all platforms out there.

Patrick