Re: [PATCH 1/2] http: add option to enable 100 Continue responses

From: Shawn Pearce <spearce@spearce.org>
Date: Oct 9, 2013, 21:19 UTC
Message-ID: <CAJo=hJvyorMjFYZnVwz4iZr88ewor6LuqOE-mpt4LsPyoddBqg@mail.gmail.com>
In-Reply-To: <20131009193054.GA3767@sigill.intra.peff.net>

On Wed, Oct 9, 2013 at 12:30 PM, Jeff King <peff@peff.net> wrote:

Show 22 quoted lines

> On Tue, Oct 08, 2013 at 08:48:06PM +0000, brian m. carlson wrote:
>
>> When using GSS-Negotiate authentication with libcurl, the authentication
>> provided will change every time, and so the probe that git uses to determine if
>> authentication is needed is not sufficient to guarantee that data can be sent.
>> If the data fits entirely in http.postBuffer bytes, the data can be rewound and
>> resent if authentication fails; otherwise, a 100 Continue must be requested in
>> this case.
>>
>> By default, curl will send an Expect: 100-continue if a certain amount of data
>> is to be uploaded, but when using chunked data this is never triggered.  Add an
>> option http.continue, which defaults to enabled, to control whether this header
>> is sent.  The addition of an option is necessary because some proxies break
>> badly if sent this header.
>
> [+cc spearce]
>
> I'd be more comfortable defaulting this to "on" if I understood more
> about the original problem that led to 959dfcf and 206b099. It sounds
> like enabling this all the time will cause annoying stalls in the
> protocol, unless the number of non-crappy proxy implementations has
> gotten smaller over the past few years.

It actually hasn't, not significantly.

206b099 was written because the Google web servers for android.googlesource.com and code.google.com do not support 100-continue semantics. This caused the client to stall a full 1 second before each POST exchange. If ancestor negotiation required O(128) have lines to be advertised I think this was 2 or 4 POSTs, resulting in 2-4 second stalls above the other latency of the network and the server.

The advice I received from the Google web server developers was to not rely on 100-continue, because a large number of corporate proxy servers do not support it correctly. HTTP 100-continue is just pretty broken in the wild.

If "Expect: 100-continue" is required for GSS-Negotiate to work then Git should only enable the option if the server is demanding GSS-Negotiate for authentication. Per 206b099 the default should still be off for anonymous and HTTP basic, digest, and SSL certificate authentication.

Show 20 quoted lines

>> diff --git a/remote-curl.c b/remote-curl.c
>> index b5ebe01..3b5e160 100644
>> --- a/remote-curl.c
>> +++ b/remote-curl.c
>> @@ -470,7 +470,12 @@ static int post_rpc(struct rpc_state *rpc)
>>
>>       headers = curl_slist_append(headers, rpc->hdr_content_type);
>>       headers = curl_slist_append(headers, rpc->hdr_accept);
>> -     headers = curl_slist_append(headers, "Expect:");
>> +
>> +     /* Force it either on or off, since curl will try to decide based on how
>> +      * much data is to be uploaded and we want consistency.
>> +      */
>> +     headers = curl_slist_append(headers, http_use_100_continue ?
>> +             "Expect: 100-continue" : "Expect:");
>
> Is there any point in sending the Expect: header in cases where curl
> would not send it, though? It seems like we should assume curl does the
> right thing most of the time, and have our option only be to override
> curl in the negative direction.

Adding a header of "Expect:" causes curl to disable the header and never use it. Always supplying the header with no value prevents libcurl from using 100-continue on its own, which is what I fixed in 959dfcf.