On Sun, Oct 26, 2014 at 8:39 AM, Dennis Kaarsemaker
<dennis@xxxxxxxxxxxxxxx> wrote:
> On Wed, Oct 22, 2014 at 10:11:40AM -0700, Junio C Hamano wrote:
>> Dennis Kaarsemaker <dennis@xxxxxxxxxxxxxxx> writes:
>>
>> > On di, 2014-10-21 at 10:56 -0700, Junio C Hamano wrote:
>> >> Dennis Kaarsemaker <dennis@xxxxxxxxxxxxxxx> writes:
>> >>
>> >> > By not clearing the request buffer in stateless-rpc mode, fetch-pack
>> >> > would keep sending already known-common commits, leading to ever bigger
>> >> > http requests, eventually getting too large for git-http-backend to
>> >> > handle properly without filling up the pipe buffer in inflate_request.
>> >> > ---
>> >> > I'm still not quite sure whether this is the right thing to do, but make
>> >> > test still passes :) The new testcase demonstrates the problem, when
>> >> > running t5551 with EXPENSIVE, this test will hang without the patch to
>> >> > fetch-pack.c and succeed otherwise.
>> >>
>> >> IIUC, because "stateless" is just that, i.e. the server-end does not
>> >> keep track of what is already known, not telling what is known to be
>> >> common in each request would fundamentally break the protocol. Am I
>> >> mistaken?
>> >
>> > That sounds plausible, but why then does the fetch complete with this
>> > line removed, and why does 'make test' still pass?
>>
>> The fetch-pack program tries to help the upload-pack program(s)
>> running on the other end find what nodes in the graph both
>> repositories have in common by sending what the repository on its
>> end has. Some commits may not be known by the other side (e.g. your
>> new commits that haven't been pushed there that are made on a branch
>> forked from the common history), and some others may be known
>> (i.e. you drilled down the history from the tips of your refs and
>> reached a commit that you fetched from the common history
>> previously). The latter are ACKed by the upload-pack process and
>> are remembered to be re-sent to the _next_ incarnation of the
>> upload-pack process when stateless RPC is in use.
>>
>> With your patch, you stop telling the upload-pack process what these
>> two programs already found to be common in their exchange. After
>> the first exchange, fetch-pack and upload-pack may have noticed that
>> both ends have version 2.0, but because you do not convey that fact
>> to the other side, the new incarnation of upload-pack may end up
>> deciding that the version 1.9 is the newest common commit between
>> the two, and sending commits between 1.9 and 2.0.
>>
>> If you imagine an extreme case, it would be easy to see why "the
>> fetch completes" and "make test passes" are not sufficient to say
>> anything about this change. Even if you break the protocol in in a
>> way different from your patch, by not sending any "have", such a
>> butchered "fetch-pack" will become "fetch everything from scratch",
>> aka "clone". The end result will still have correct history and
>> "fetch completes" would be true.
>>
>> But I'd prefer deferring a more detailed analysis/explanation to
>> Shawn, as stateless RPC is his creation.
Junio's statement above about the world is correct.
> Thanks for the explanation, that makes it quite clear that this approach
> is wrong. The patch below (apologies for the formatting, I'm not quite
> sure how to use format-patch in this situation) does it differently: by
> buffering upload-pack's output until it has read all the input, the new
> test still succeeds and again 'make test' passes.
This probably introduces latency into the traditional bidirectional
multi_ack protocol.
> @@ -384,15 +385,19 @@ static int get_common_commits(void)
> if (multi_ack == 2 && got_common
> && !got_other && ok_to_give_up()) {
> sent_ready = 1;
> - packet_write(1, "ACK %s ready\n", last_hex);
> + packet_buf_write(&resp_buf, "ACK %s ready\n", last_hex);
> }
> if (have_obj.nr == 0 || multi_ack)
> - packet_write(1, "NAK\n");
> + packet_buf_write(&resp_buf, "NAK\n");
By buffering and delaying these when !stateless_rpc we defer telling
the peer in a multi_ack exchange. That delays the peer from cutting
off its communication by about 1RTT.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html