Re: [PATCHv2 2/2] receive-pack: Add receive.objectCountLimit to refuse push with too many objects

From: Shawn Pearce <spearce@spearce.org>
Date: May 14, 2011, 22:17 UTC
Message-ID: <BANLkTimQD5JFeOeF48cbMjDL3+WMfsmPtA@mail.gmail.com>
In-Reply-To: <201105141517.45324.johan@herland.net>

On Sat, May 14, 2011 at 06:17, Johan Herland <johan@herland.net> wrote:

Show 9 quoted lines

> I'm planning to add a new capability collection/namespace, called "limit-*",
> where the server can communicate capabilities to the client, like so:
>
>  limit-object-count_100000
>  limit-commit-count_1000
>  limit-pack-size_500000000
>
> (I'd prefer to s/_/=/ or s/_/:/, but according to pack-protocol.txt, a
> capability may not contain "=" or ":")

I forget why = and : are forbidden here. I think its just because we wanted the options to be "simple". I agree, I would prefer = here too, and probably would have written the patch that way myself. There shouldn't be a technical reason why = isn't allowed here. Its just documented as being not a good idea because at one time someone wrote that down.

Show 7 quoted lines

> However, you say:
>
>> For older clients that don't know this new advertised capability, they
>> should fail hard and not transfer all of this data.
>
> AFAICS this is not the case. If a client does not understand a capability,
> it simply ignores it, and carries on doing its usual thing.

By this I meant #2 below (the initial patch).

Show 5 quoted lines

> IINM there are only two ways to prevent an older client from transferring
> all the data:
>
> 1. Change the pack protocol in an incompatible way, that causes older client
> to abort with a pack format error prior to transmitting the pack.

This is not a good idea. We still want the client to be able to talk to the server if it would be within the limits.

> 2. (as in initial patch) Abort receive-pack when the server detects a limit
> violation, leaving the client with a broken pipe. I haven't read the pack
> protocol closely, but I wouldn't be surprised if this behavior is strictly
> in violation of the protocol.

It is a violation of the protocol... sort of. Its allowed for the server to up and die in the middle of a push. What happens if the remote system loses power due to a grid failure while you are writing to it? The remote system can't tell you "I'm going away now" first. It just freezes and stops ACK'ing the TCP packets. Or if the remote system gets overloaded and the Linux OOM killer kicks in... the remote might get one of the processes elected for killing, and your TCP connection breaks.

I don't think its as bad as it sounds. Its not a great user experience, sure. And we maybe should also look at changing the send-pack code to check the pipe for received data from the remote peer if pack-objects dies (today it doesn't)... just in case the reason pack-objects died is because an error message was written and then the stream was closed.

-- 
Shawn.