You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by iv...@apache.org on 2013/06/21 14:47:46 UTC
svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Author: ivan
Date: Fri Jun 21 12:47:46 2013
New Revision: 1495419
URL: http://svn.apache.org/r1495419
Log:
Rework server chunked transfer encoding support detection in ra_serf. Use
HTTP/1.1 chunked transfer encoding for first OPTIONS request and then
fallback to HTTP/1.0 requests if HTTP 411 status code or HTTP/1.0 response
received.
Discussion and report: http://svn.haxx.se/dev/archive-2013-06/0408.shtml
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__session_t): Remove HTTP10 and add USE_CHUNKED_ENCODING.
* subversion/libsvn_ra_serf/options.c
(svn_ra_serf__exchange_capabilities): Retry OPTIONS request without using
chunked transfer encoding if HTTP 411 status code or HTTP/1.0 response
received for chunked request.
* subversion/libsvn_ra_serf/serf.c
(svn_ra_serf__open): Initialize USE_CHUNKED_ENCODING to TRUE.
* subversion/libsvn_ra_serf/util.c
(setup_serf_req): Use USE_CHUNKED_ENCODING member instead of removed
HTTP10. Always send 'Connection: keep-alive' header.
Modified:
subversion/trunk/subversion/libsvn_ra_serf/options.c
subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
subversion/trunk/subversion/libsvn_ra_serf/serf.c
subversion/trunk/subversion/libsvn_ra_serf/util.c
Modified: subversion/trunk/subversion/libsvn_ra_serf/options.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/options.c?rev=1495419&r1=1495418&r2=1495419&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/options.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/options.c Fri Jun 21 12:47:46 2013
@@ -488,6 +488,22 @@ svn_ra_serf__exchange_capabilities(svn_r
err = svn_ra_serf__context_run_one(opt_ctx->handler, pool);
+ /* Retry request if HTTP/1.1 411 Length Required or we got HTTP/1.0 response. */
+ if (serf_sess->use_chunked_encoding &&
+ (opt_ctx->handler->sline.code == 411 ||
+ opt_ctx->handler->sline.version == SERF_HTTP_10))
+ {
+ /* Ignore any errors and retry request using HTTP/1.0 with
+ Content-Length.*/
+ svn_error_clear(err);
+
+ serf_sess->use_chunked_encoding = FALSE;
+
+ SVN_ERR(create_options_req(&opt_ctx, serf_sess, serf_sess->conns[0], pool));
+
+ err = svn_ra_serf__context_run_one(opt_ctx->handler, pool);
+ }
+
/* If our caller cares about server redirections, and our response
carries such a thing, report as much. We'll disregard ERR --
it's most likely just a complaint about the response body not
Modified: subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h?rev=1495419&r1=1495418&r2=1495419&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h Fri Jun 21 12:47:46 2013
@@ -140,9 +140,8 @@ struct svn_ra_serf__session_t {
apr_uri_t repos_root;
const char *repos_root_str;
- /* The server is not Apache/mod_dav_svn (directly) and only supports
- HTTP/1.0. Thus, we cannot send chunked requests. */
- svn_boolean_t http10;
+ /* The server supports chunked request bodies. */
+ svn_boolean_t use_chunked_encoding;
/* Our Version-Controlled-Configuration; may be NULL until we know it. */
const char *vcc_url;
Modified: subversion/trunk/subversion/libsvn_ra_serf/serf.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/serf.c?rev=1495419&r1=1495418&r2=1495419&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/serf.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/serf.c Fri Jun 21 12:47:46 2013
@@ -437,9 +437,10 @@ svn_ra_serf__open(svn_ra_session_t *sess
serf_sess->capabilities = apr_hash_make(serf_sess->pool);
- /* We have to assume that the server only supports HTTP/1.0. Once it's clear
- HTTP/1.1 is supported, we can upgrade. */
- serf_sess->http10 = TRUE;
+ /* Assume HTTP/1.1 server and use chunked transfer encoding. We fallback
+ * to HTTP/1.0 requests in svn_ra_serf__exchange_capabilities() if server
+ * doesn't support chunked encoding. */
+ serf_sess->use_chunked_encoding = FALSE;
SVN_ERR(load_config(serf_sess, config, serf_sess->pool));
Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1495419&r1=1495418&r2=1495419&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/util.c Fri Jun 21 12:47:46 2013
@@ -642,7 +642,7 @@ setup_serf_req(serf_request_t *request,
svn_spillbuf_t *buf;
- if (session->http10 && body_bkt != NULL)
+ if (!session->use_chunked_encoding && body_bkt != NULL)
{
/* Ugh. Use HTTP/1.0 to talk to the server because we don't know if
it speaks HTTP/1.1 (and thus, chunked requests), or because the
@@ -670,7 +670,7 @@ setup_serf_req(serf_request_t *request,
/* Set the Content-Length value. This will also trigger an HTTP/1.0
request (rather than the default chunked request). */
- if (session->http10)
+ if (!session->use_chunked_encoding)
{
if (body_bkt == NULL)
serf_bucket_request_set_CL(*req_bkt, 0);
@@ -690,10 +690,9 @@ setup_serf_req(serf_request_t *request,
serf_bucket_headers_setn(*hdrs_bkt, "Content-Type", content_type);
}
- if (session->http10)
- {
- serf_bucket_headers_setn(*hdrs_bkt, "Connection", "keep-alive");
- }
+ /* Always set Connection: keep-alive because we don't know if server
+ * is HTTP/1.1 aware. */
+ serf_bucket_headers_setn(*hdrs_bkt, "Connection", "keep-alive");
if (accept_encoding)
{
@@ -1861,10 +1860,6 @@ handle_response(serf_request_t *request,
handler->sline = sl;
handler->sline.reason = apr_pstrdup(handler->handler_pool, sl.reason);
-
- /* HTTP/1.1? (or later) */
- if (sl.version != SERF_HTTP_10)
- handler->session->http10 = FALSE;
}
/* Keep reading from the network until we've read all the headers. */
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Jun 21, 2013 at 9:51 AM, Greg Stein <gs...@gmail.com> wrote:
>...
> But the answer is *not* to start a connection using 1.0.
1.1, of course. Start with 1.0, then upgrade.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c
ra_serf.h serf.c util.c
Posted by km...@rockwellcollins.com.
> > On Jun 21, 2013 10:36 AM, "Ivan Zhakov" <iv...@visualsvn.com> wrote:
> > >....
> > > >> Problem with this approach that some servers may support HTTP/1.1
> > > >> partially. I.e. declare them as HTTP/1.1 but do not support
chunked
> > > >> Transfer-Encoding.
> > > >
> > > > Then fix that problem. Add a flag saying "busted_http11" and make
it
> > > > send requests with Content-Length after that.
> > > >
> > > Do you mean run-time configuration option like "http-force-http10 =
yes"?
> > Oh. Hadn't thought about that. I was thinking dynamic detection.
> > I would favor dynamic over Yet Another Config Option, but you're the
> > one writing this code right now :-)
>
> Dynamic is the right approach because the spec does allow a 1.1 proxy
> to return a 411 to a chunk encoded request:
>
>
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-22#section-3.3.3
>
> (This will obsolete 2145 and 2616 if/when approved)
>
> This is validated (albeit discouraged) by the the chair of that working
group:
>
> http://www.mnot.net/blog/2011/07/11/what_proxies_must_do
The httpbis spec further says:
Unless a transfer coding other than chunked has been applied, a
client that sends a request containing a message body SHOULD use a
valid Content-Length header field if the message body length is known
in advance, rather than the chunked transfer coding, since some
existing services respond to chunked with a 411 (Length Required)
status code even though they understand the chunked transfer coding.
This is typically because such services are implemented via a gateway
that requires a content-length in advance of being called and the
server is unable or unwilling to buffer the entire request before
processing.
I haven't looked at the serf code, but I would assume most request
message bodies would have a known size, so serf really should be
preferring to send content-length instead of chunked encoding
for request messages.
Kevin R.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c
ra_serf.h serf.c util.c
Posted by km...@rockwellcollins.com.
> On Jun 21, 2013 10:36 AM, "Ivan Zhakov" <iv...@visualsvn.com> wrote:
> >....
> > >> Problem with this approach that some servers may support HTTP/1.1
> > >> partially. I.e. declare them as HTTP/1.1 but do not support chunked
> > >> Transfer-Encoding.
> > >
> > > Then fix that problem. Add a flag saying "busted_http11" and make it
> > > send requests with Content-Length after that.
> > >
> > Do you mean run-time configuration option like "http-force-http10 =
yes"?
> Oh. Hadn't thought about that. I was thinking dynamic detection.
> I would favor dynamic over Yet Another Config Option, but you're the
> one writing this code right now :-)
Dynamic is the right approach because the spec does allow a 1.1 proxy
to return a 411 to a chunk encoded request:
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-22#section-3.3.3
(This will obsolete 2145 and 2616 if/when approved)
This is validated (albeit discouraged) by the the chair of that working
group:
http://www.mnot.net/blog/2011/07/11/what_proxies_must_do
Kevin R.
Re: svn commit: r1495419 - in
/subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c
util.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Jun 26, 2013 at 12:36:22AM +0200, Lieven Govaerts wrote:
> What you say is not correct, the reason has been explained earlier in
> this thread:
> http://svn.haxx.se/dev/archive-2013-06/0530.shtml
I'm OK with making the extra request conditional on a config knob.
However, as you said in the linked post, we must make sure that the
error message displayed clearly asks users to set the knob and try again.
Working around broken proxies will turn into a support nightmare in
many organistations, even with a clear error message (cause most users
don't really read or understand our error messages ;) But having a clear
error message should help many users a lot.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Lieven Govaerts <lg...@mobsol.be>.
On Wed, Jun 26, 2013 at 12:24 AM, Daniel Shahaf <da...@elego.de> wrote:
> Ivan Zhakov wrote on Wed, Jun 26, 2013 at 02:21:05 +0400:
>> On Wed, Jun 26, 2013 at 2:12 AM, Daniel Shahaf <da...@elego.de> wrote:
>> > Johan Corveleyn wrote on Tue, Jun 25, 2013 at 23:22:12 +0200:
>> >> On Tue, Jun 25, 2013 at 11:03 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >> > On Tue, Jun 25, 2013 at 10:45 PM, Greg Stein <gs...@gmail.com> wrote:
>> >> >> On Tue, Jun 25, 2013 at 11:55 AM, Philip Martin
>> >> >> <ph...@wandisco.com> wrote:
>> >> >>> Branko Čibej <br...@wandisco.com> writes:
>> >> >>>
>> >> >>>> I'm really not a fan of this config knob. Anyone who carries their
>> >> >>>> laptop around will effectively have to set this as the default, because
>> >> >>>> you never know when the next weird proxy will pop up in front of your
>> >> >>>> server. And disabling chunked requests by default is a lot worse than
>> >> >>>> the extra non-pipelined request for broken proxies, IMO.
>> >> >>
>> >> >> Right.
>> >> >>
>> >> >> Though I suspect most of the problems are reverse proxies in front of
>> >> >> a particular server, so you can put the config option into a [server]
>> >> >> config block instead of global. That will help to limit the problem,
>> >> >> but lack of dynamic detection is still a problem.
>> >> >>
>> >> > What is the benefit of dynamic detection enabled by some knob in config file?
>> >>
>> >> The dynamic detection has a cost (1 extra request per connection),
>> >> that you might want to avoid by default (most environments won't need
>> >> the dynamic detection (especially corporate environments)). Only
>> >> enable the dynamic detection if you know the proxy has a problem with
>> >> chunkness, or if you're not sure it will stay that way, or ...
>> >>
>> >> (not interfering with the rest of the discussion right now :-)
>> >
>> > AIUI the cost is only incurred by set-ups that have the so-called
>> > "busted" proxies. And a config option has a cost too: it would need to
>> > be supported until 2.0 (aka, indefinitely).
>> Please note that this extra request is per session and currently we
>> create many sessions even during one operation. And I'm also not happy
>> to make performance worse for users who doesn't use reverse proxies
>> and etc.
>
> Please define "etc".
>
> Also, I just said that the cost is only incurred only by people who use
> a "so-called 'busted' proxy". If you think that is not true, please say
> that explicitly, I don't want to have to fish from your words whether
> you think that is the case or not.
What you say is not correct, the reason has been explained earlier in
this thread:
http://svn.haxx.se/dev/archive-2013-06/0530.shtml
> (We have enough bad implications on IRC right now; don't need more on list)
Re: svn commit: r1495419 - in
/subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h
serf.c util.c
Posted by Daniel Shahaf <da...@elego.de>.
Ivan Zhakov wrote on Wed, Jun 26, 2013 at 02:21:05 +0400:
> On Wed, Jun 26, 2013 at 2:12 AM, Daniel Shahaf <da...@elego.de> wrote:
> > Johan Corveleyn wrote on Tue, Jun 25, 2013 at 23:22:12 +0200:
> >> On Tue, Jun 25, 2013 at 11:03 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >> > On Tue, Jun 25, 2013 at 10:45 PM, Greg Stein <gs...@gmail.com> wrote:
> >> >> On Tue, Jun 25, 2013 at 11:55 AM, Philip Martin
> >> >> <ph...@wandisco.com> wrote:
> >> >>> Branko Čibej <br...@wandisco.com> writes:
> >> >>>
> >> >>>> I'm really not a fan of this config knob. Anyone who carries their
> >> >>>> laptop around will effectively have to set this as the default, because
> >> >>>> you never know when the next weird proxy will pop up in front of your
> >> >>>> server. And disabling chunked requests by default is a lot worse than
> >> >>>> the extra non-pipelined request for broken proxies, IMO.
> >> >>
> >> >> Right.
> >> >>
> >> >> Though I suspect most of the problems are reverse proxies in front of
> >> >> a particular server, so you can put the config option into a [server]
> >> >> config block instead of global. That will help to limit the problem,
> >> >> but lack of dynamic detection is still a problem.
> >> >>
> >> > What is the benefit of dynamic detection enabled by some knob in config file?
> >>
> >> The dynamic detection has a cost (1 extra request per connection),
> >> that you might want to avoid by default (most environments won't need
> >> the dynamic detection (especially corporate environments)). Only
> >> enable the dynamic detection if you know the proxy has a problem with
> >> chunkness, or if you're not sure it will stay that way, or ...
> >>
> >> (not interfering with the rest of the discussion right now :-)
> >
> > AIUI the cost is only incurred by set-ups that have the so-called
> > "busted" proxies. And a config option has a cost too: it would need to
> > be supported until 2.0 (aka, indefinitely).
> Please note that this extra request is per session and currently we
> create many sessions even during one operation. And I'm also not happy
> to make performance worse for users who doesn't use reverse proxies
> and etc.
Please define "etc".
Also, I just said that the cost is only incurred only by people who use
a "so-called 'busted' proxy". If you think that is not true, please say
that explicitly, I don't want to have to fish from your words whether
you think that is the case or not.
(We have enough bad implications on IRC right now; don't need more on list)
Re: svn commit: r1495419 - in
/subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h
serf.c util.c
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Tue, Jun 25, 2013 at 22:19:30 +0200:
> On Tue, Jun 25, 2013 at 09:15:59PM +0200, Branko Čibej wrote:
> > On 25.06.2013 20:45, Greg Stein wrote:
> > > To that end, I find r1496470 to be insufficient. We need the dynamic
> > > stuff, and we need to fix the configuration option name. I'm happy to
> > > work on these two pieces, if we have a bit of consensus.
> >
> > We (that is, you and I) certainly do. Is that a quorum? :)
>
> I'd also prefer dynamic detection, and no config knob at all if possible.
+1
Re: svn commit: r1495419 - in
/subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c
util.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Jun 25, 2013 at 09:15:59PM +0200, Branko Čibej wrote:
> On 25.06.2013 20:45, Greg Stein wrote:
> > To that end, I find r1496470 to be insufficient. We need the dynamic
> > stuff, and we need to fix the configuration option name. I'm happy to
> > work on these two pieces, if we have a bit of consensus.
>
> We (that is, you and I) certainly do. Is that a quorum? :)
I'd also prefer dynamic detection, and no config knob at all if possible.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jun 25, 2013 at 6:21 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Wed, Jun 26, 2013 at 2:12 AM, Daniel Shahaf <da...@elego.de> wrote:
>> Johan Corveleyn wrote on Tue, Jun 25, 2013 at 23:22:12 +0200:
>...
>>> The dynamic detection has a cost (1 extra request per connection),
>>> that you might want to avoid by default (most environments won't need
>>> the dynamic detection (especially corporate environments)). Only
>>> enable the dynamic detection if you know the proxy has a problem with
>>> chunkness, or if you're not sure it will stay that way, or ...
Right. My intent is a config option to enable the extra request when
problems may be present.
>...
>> AIUI the cost is only incurred by set-ups that have the so-called
>> "busted" proxies. And a config option has a cost too: it would need to
>> be supported until 2.0 (aka, indefinitely).
Right.
> Please note that this extra request is per session and currently we
> create many sessions even during one operation. And I'm also not happy
> to make performance worse for users who doesn't use reverse proxies
> and etc.
People with busted proxies will just need to deal with it. And
libsvn_client should work on using fewer sessions.
Most people will never enable the config, and never have a problem or
reduced performance.
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Wed, Jun 26, 2013 at 2:12 AM, Daniel Shahaf <da...@elego.de> wrote:
> Johan Corveleyn wrote on Tue, Jun 25, 2013 at 23:22:12 +0200:
>> On Tue, Jun 25, 2013 at 11:03 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> > On Tue, Jun 25, 2013 at 10:45 PM, Greg Stein <gs...@gmail.com> wrote:
>> >> On Tue, Jun 25, 2013 at 11:55 AM, Philip Martin
>> >> <ph...@wandisco.com> wrote:
>> >>> Branko Čibej <br...@wandisco.com> writes:
>> >>>
>> >>>> I'm really not a fan of this config knob. Anyone who carries their
>> >>>> laptop around will effectively have to set this as the default, because
>> >>>> you never know when the next weird proxy will pop up in front of your
>> >>>> server. And disabling chunked requests by default is a lot worse than
>> >>>> the extra non-pipelined request for broken proxies, IMO.
>> >>
>> >> Right.
>> >>
>> >> Though I suspect most of the problems are reverse proxies in front of
>> >> a particular server, so you can put the config option into a [server]
>> >> config block instead of global. That will help to limit the problem,
>> >> but lack of dynamic detection is still a problem.
>> >>
>> > What is the benefit of dynamic detection enabled by some knob in config file?
>>
>> The dynamic detection has a cost (1 extra request per connection),
>> that you might want to avoid by default (most environments won't need
>> the dynamic detection (especially corporate environments)). Only
>> enable the dynamic detection if you know the proxy has a problem with
>> chunkness, or if you're not sure it will stay that way, or ...
>>
>> (not interfering with the rest of the discussion right now :-)
>
> AIUI the cost is only incurred by set-ups that have the so-called
> "busted" proxies. And a config option has a cost too: it would need to
> be supported until 2.0 (aka, indefinitely).
Please note that this extra request is per session and currently we
create many sessions even during one operation. And I'm also not happy
to make performance worse for users who doesn't use reverse proxies
and etc.
--
Ivan Zhakov
Re: svn commit: r1495419 - in
/subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h
serf.c util.c
Posted by Daniel Shahaf <da...@elego.de>.
Johan Corveleyn wrote on Tue, Jun 25, 2013 at 23:22:12 +0200:
> On Tue, Jun 25, 2013 at 11:03 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> > On Tue, Jun 25, 2013 at 10:45 PM, Greg Stein <gs...@gmail.com> wrote:
> >> On Tue, Jun 25, 2013 at 11:55 AM, Philip Martin
> >> <ph...@wandisco.com> wrote:
> >>> Branko Čibej <br...@wandisco.com> writes:
> >>>
> >>>> I'm really not a fan of this config knob. Anyone who carries their
> >>>> laptop around will effectively have to set this as the default, because
> >>>> you never know when the next weird proxy will pop up in front of your
> >>>> server. And disabling chunked requests by default is a lot worse than
> >>>> the extra non-pipelined request for broken proxies, IMO.
> >>
> >> Right.
> >>
> >> Though I suspect most of the problems are reverse proxies in front of
> >> a particular server, so you can put the config option into a [server]
> >> config block instead of global. That will help to limit the problem,
> >> but lack of dynamic detection is still a problem.
> >>
> > What is the benefit of dynamic detection enabled by some knob in config file?
>
> The dynamic detection has a cost (1 extra request per connection),
> that you might want to avoid by default (most environments won't need
> the dynamic detection (especially corporate environments)). Only
> enable the dynamic detection if you know the proxy has a problem with
> chunkness, or if you're not sure it will stay that way, or ...
>
> (not interfering with the rest of the discussion right now :-)
AIUI the cost is only incurred by set-ups that have the so-called
"busted" proxies. And a config option has a cost too: it would need to
be supported until 2.0 (aka, indefinitely).
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Lieven Govaerts <li...@gmail.com>.
On Tue, Jul 9, 2013 at 7:33 PM, Ben Reser <be...@reser.org> wrote:
> On Tue, Jul 9, 2013 at 1:25 AM, Stefan Sperling <st...@elego.de> wrote:
>> On Tue, Jul 09, 2013 at 12:45:49AM -0700, Ben Reser wrote:
>>> The option doesn't seem like a big barrier when you're applying it to
>>> a handful of machines for yourself. Scale that effort up to a user
>>> base of 20,000 users and the option stops looking like a reasonable
>>> response. Resolving the proxy may not be possible in those cases as
>>> well because the developers and the people running the Subversion
>>> servers may have absolutely no control over the proxies. They may be
>>> proprietary and not even have a way to resolve this. If you're a
>>> large enterprise this solution just flat out doesn't help you much.
>>
>> I think this is a very important point.
>>
>> How many people use auto-props? Not many. Still, having auto-props
>> configured at the client side made configuring this feature correctly
>> in large deployments very inconvenient. Until 1.8 it requried central
>> control over every client configuration. This was a concern in several
>> companies I've visited.
>>
>> How many people are using proxies that don't support chunked requests?
>> Probably not many. But the same argument applies.
>>
>>> I agree that chunked requests are very good for us. But I want to do
>>> it in a way that is maximally compatible without making our users jump
>>> through hoops.
>>
>> +1
>>
>> With so many other performance issues being fixed all the time,
>> I don't think we should be worried about one extra request.
>> We should be able to get the time spent on this extra request back
>> somewhere else.
>>
>> 'svn merge' in 1.8 opens more connections that 1.7 did, and that is
>> a performance hit but it didn't prevent the 1.8 release either.
>
> So we had a conversation on IRC this morning about solutions.
>
> This is what several of us seemed to agree was a reasonable compromise:
>
> Have a tri-state option called http-chunked-requests. It would have
> three states:
> auto = Run the extra OPTIONS request and detect if chunked requests is
> supported or not.
> yes = Assume that HTTP/1.1 servers have chunked requests support (our
> 1.8.0 default behavior) and always use it against HTTP/1.1 servers.
> no = Never use chunked requests.
>
> With the default being auto.
First: I prefer the solution currently merged to the 1.8.1 branch
(fail first, tell the user to change the busted-proxy option, then
automatically detect), because of two advantages over your proposed
solution:
a. It doesn't impact those users that have no 'busted' proxies between
them and their repositories.
b. It gives a sign that a busted or at the minimum less optimal proxy
is being used. We have seen over the last week that the latest
versions of the nginx proxy support chunked encoding for requests
without problems, and that sometimes it's just a question of reporting
this issue for an admin to take action.
Second: The basic decision to take here is if we want to give all
users a transparent solution to connect to any svn server, whatever
proxy being used. Do we want to bother a user with such technical
details as busted proxies?
Personally I can live with that, given that svn 1.8.1-branch now has a
simple workaround and is (reasonably) clear in its communication about
it with the 411 error message.
Now, if there's consensus between svn devs that the highest priority
for the solution is transparency for the user then I'll accept that,
at least for the 1.8.x line.
> This provides us a lot of flexibility in handling this. With a
> tri-state we can reasonably change the default behavior if the proxy
> environment changes or we find the auto detection to be too costly in
> the wild. If we find a better way of automatically handling this we
> can change what auto does.
>
> This avoids the need for users to set an option in order to
> interoperate, but users that want maximum performance can set it to
> yes (assuming they don't have such a proxy in line).
>
> Thoughts?
Suppose that this is the consensus, then honestly I don't see a reason
why we would add the http-chunked-requests option.
Auto we don't need because that's what's implemented, and we can
always change that even without option. Yes and No will probably used
by no one expect for the few people that have followed this
discussion. Then I rather have no option at all, ra_serf now already
has too many different combinations in which it can be used
(http/https,v1/v2,bulk,authn,http1.0/1.1,chunked encoding,...) for us
to test and support.
Lieven
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jul 10, 2013 at 7:50 AM, Johan Corveleyn <jc...@gmail.com> wrote:
> On Wed, Jul 10, 2013 at 1:24 PM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Johan Corveleyn <jc...@gmail.com> writes:
>>
>>> Can someone explain again why ra_serf / serf can't just resend
>>> requests, with C-L, whenever it has received a 411? So (after we know
>>> it's HTTP/1.1) send every request optimistically assuming chunked
>>> encoding, and fallback whenever we get a 411 (and perhaps remember the
>>> fallback, so we downgrade that entire connection / session).
>>>
>>> Why is that not possible? Is it because (a) it's too hard to implement
>>> (lots of code paths all over the place), (b) we can't resend requests
>>> because we don't have them available in the right place anymore after
>>> sending, (c) it would potentially break some ordering things (really?
>>> why?), (d) it would require serf 1.4 anyway, or (e) something else?
>>
>> Partly a), mostly c). It is relatively easy to do when ra_serf is
>> processing requests syncronously. It gets much more difficult when
>> ra_serf starts using pipelined requests as there may be outstanding
>> requests when the 411 is received. I posted a patch that keeps track of
>> outstanding requests and retries, it works in lots of cases but won't
>> work in all cases.
>
> Is pipelining optional within (ra-)serf's logic? I.e. can the "start
> pipelining" switch simply be flipped a little bit later, once we're
> sure that chunking works?
Yes. And funny you mention it because I was saying the same thing to
Ben on IRC last night.
However: we cannot do that in a simple fashion, such that I would be
comfortable suggesting a backport to 1.8.x.
Consider if we adopted the "auto" approach and generally sent an extra
OPTIONS request. In the 1.9.x series, we could start to optimize that
away by using the next synchronous request instead. Or when we hit
your question (b), if a body is hard to reproduce, then we hold that
for a moment and send the OPTIONS. And we could disable pipelining for
one request, while we probe the connection's level of support.
> That way we won't do an extra (synchronous,
> but useless) request just for detecting chunkness, but we just
> continue working synchronously with our normal (useful) requests a
> tiny bit longer, before switching to pipelined mode. Still a small
> performance hit in general, but it might be negligible.
Right. And something very doable for 1.9.x. I also want to implement
request compression. That will help some during an import (where we
self-diff the imported file, rather than sending a diff-against-BASE).
An analysis by ghudson long ago showed that our self-diff is actually
pretty good compared to gzip, but I'd prefer to use optimized gzip at
both ends instead of our algorithm.
> Totally making abstraction of implementation complexity, of course ...
>
> Of course, if you could make your patch work in all cases, Philip,
> that would be even better :-).
The ra_serf stuff isn't too bad. Much of the request setup/handling is
in util.c. But we have about three different run loops right now
(one-shot/synchronous, update, and (iirc) replay). They optimize the
connection(s) in different ways.
When we switch to Ev2, there will be some great optimizations in
commit. In particular, alter_file(CONTENTS) means that our
callback-from-serf can read the stream. No transient spillbuf/file
necessary (compared to Ev1, where apply_textdelta pushes content
*into* the RA commit process).
Within 1.9.x, we also can switch more temp-file usage over to use the
spillbuf bucket (ref: sb_bucket.c). That'll keep small items in memory
rather than forced-to-disk.
But back to the original point: what is the smallest patch that we're
comfortable with now? From a user standpoint, and a backport-safety
standpoint. The "auto" suggestion is likely our best solution for now.
Ben put together a branch, but I think it's more complicated than
necessary. That's fixable though :-)
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Johan Corveleyn <jc...@gmail.com>.
On Wed, Jul 10, 2013 at 1:24 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Johan Corveleyn <jc...@gmail.com> writes:
>
>> Can someone explain again why ra_serf / serf can't just resend
>> requests, with C-L, whenever it has received a 411? So (after we know
>> it's HTTP/1.1) send every request optimistically assuming chunked
>> encoding, and fallback whenever we get a 411 (and perhaps remember the
>> fallback, so we downgrade that entire connection / session).
>>
>> Why is that not possible? Is it because (a) it's too hard to implement
>> (lots of code paths all over the place), (b) we can't resend requests
>> because we don't have them available in the right place anymore after
>> sending, (c) it would potentially break some ordering things (really?
>> why?), (d) it would require serf 1.4 anyway, or (e) something else?
>
> Partly a), mostly c). It is relatively easy to do when ra_serf is
> processing requests syncronously. It gets much more difficult when
> ra_serf starts using pipelined requests as there may be outstanding
> requests when the 411 is received. I posted a patch that keeps track of
> outstanding requests and retries, it works in lots of cases but won't
> work in all cases.
Is pipelining optional within (ra-)serf's logic? I.e. can the "start
pipelining" switch simply be flipped a little bit later, once we're
sure that chunking works? That way we won't do an extra (synchronous,
but useless) request just for detecting chunkness, but we just
continue working synchronously with our normal (useful) requests a
tiny bit longer, before switching to pipelined mode. Still a small
performance hit in general, but it might be negligible.
Totally making abstraction of implementation complexity, of course ...
Of course, if you could make your patch work in all cases, Philip,
that would be even better :-).
--
Johan
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
Posted by Philip Martin <ph...@wandisco.com>.
Johan Corveleyn <jc...@gmail.com> writes:
> Can someone explain again why ra_serf / serf can't just resend
> requests, with C-L, whenever it has received a 411? So (after we know
> it's HTTP/1.1) send every request optimistically assuming chunked
> encoding, and fallback whenever we get a 411 (and perhaps remember the
> fallback, so we downgrade that entire connection / session).
>
> Why is that not possible? Is it because (a) it's too hard to implement
> (lots of code paths all over the place), (b) we can't resend requests
> because we don't have them available in the right place anymore after
> sending, (c) it would potentially break some ordering things (really?
> why?), (d) it would require serf 1.4 anyway, or (e) something else?
Partly a), mostly c). It is relatively easy to do when ra_serf is
processing requests syncronously. It gets much more difficult when
ra_serf starts using pipelined requests as there may be outstanding
requests when the 411 is received. I posted a patch that keeps track of
outstanding requests and retries, it works in lots of cases but won't
work in all cases.
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Jul 9, 2013 at 7:33 PM, Ben Reser <be...@reser.org> wrote:
> On Tue, Jul 9, 2013 at 1:25 AM, Stefan Sperling <st...@elego.de> wrote:
>> On Tue, Jul 09, 2013 at 12:45:49AM -0700, Ben Reser wrote:
>>> The option doesn't seem like a big barrier when you're applying it to
>>> a handful of machines for yourself. Scale that effort up to a user
>>> base of 20,000 users and the option stops looking like a reasonable
>>> response. Resolving the proxy may not be possible in those cases as
>>> well because the developers and the people running the Subversion
>>> servers may have absolutely no control over the proxies. They may be
>>> proprietary and not even have a way to resolve this. If you're a
>>> large enterprise this solution just flat out doesn't help you much.
>>
>> I think this is a very important point.
>>
>> How many people use auto-props? Not many. Still, having auto-props
>> configured at the client side made configuring this feature correctly
>> in large deployments very inconvenient. Until 1.8 it requried central
>> control over every client configuration. This was a concern in several
>> companies I've visited.
>>
>> How many people are using proxies that don't support chunked requests?
>> Probably not many. But the same argument applies.
>>
>>> I agree that chunked requests are very good for us. But I want to do
>>> it in a way that is maximally compatible without making our users jump
>>> through hoops.
>>
>> +1
>>
>> With so many other performance issues being fixed all the time,
>> I don't think we should be worried about one extra request.
>> We should be able to get the time spent on this extra request back
>> somewhere else.
>>
>> 'svn merge' in 1.8 opens more connections that 1.7 did, and that is
>> a performance hit but it didn't prevent the 1.8 release either.
>
> So we had a conversation on IRC this morning about solutions.
>
> This is what several of us seemed to agree was a reasonable compromise:
>
> Have a tri-state option called http-chunked-requests. It would have
> three states:
> auto = Run the extra OPTIONS request and detect if chunked requests is
> supported or not.
> yes = Assume that HTTP/1.1 servers have chunked requests support (our
> 1.8.0 default behavior) and always use it against HTTP/1.1 servers.
> no = Never use chunked requests.
>
> With the default being auto.
>
> This provides us a lot of flexibility in handling this. With a
> tri-state we can reasonably change the default behavior if the proxy
> environment changes or we find the auto detection to be too costly in
> the wild. If we find a better way of automatically handling this we
> can change what auto does.
>
> This avoids the need for users to set an option in order to
> interoperate, but users that want maximum performance can set it to
> yes (assuming they don't have such a proxy in line).
>
> Thoughts?
Naive question perhaps, and it may have been answered before (but with
these extremely long threads, I've lost track a bit):
Can someone explain again why ra_serf / serf can't just resend
requests, with C-L, whenever it has received a 411? So (after we know
it's HTTP/1.1) send every request optimistically assuming chunked
encoding, and fallback whenever we get a 411 (and perhaps remember the
fallback, so we downgrade that entire connection / session).
Why is that not possible? Is it because (a) it's too hard to implement
(lots of code paths all over the place), (b) we can't resend requests
because we don't have them available in the right place anymore after
sending, (c) it would potentially break some ordering things (really?
why?), (d) it would require serf 1.4 anyway, or (e) something else?
It seems to me that, if this were possible, that would be the perfect
solution (until serf 1.4 comes around perhaps). It would give no
performance penalty to the 99% ("optimistic chunking"), and we
downgrade transparently for the 1% (at some limited cost).
--
Johan
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/09/2013 10:33 AM, Ben Reser wrote:
> So we had a conversation on IRC this morning about solutions.
>
> This is what several of us seemed to agree was a reasonable compromise:
>
> Have a tri-state option called http-chunked-requests. It would have
> three states:
> auto = Run the extra OPTIONS request and detect if chunked requests is
> supported or not.
> yes = Assume that HTTP/1.1 servers have chunked requests support (our
> 1.8.0 default behavior) and always use it against HTTP/1.1 servers.
> no = Never use chunked requests.
>
> With the default being auto.
>
> This provides us a lot of flexibility in handling this. With a
> tri-state we can reasonably change the default behavior if the proxy
> environment changes or we find the auto detection to be too costly in
> the wild. If we find a better way of automatically handling this we
> can change what auto does.
>
> This avoids the need for users to set an option in order to
> interoperate, but users that want maximum performance can set it to
> yes (assuming they don't have such a proxy in line).
>
> Thoughts?
First, I appreciate the time and attention that is being invested into a
workable solution here that recognizes the real challenges of pleasing
our humongous user base. We've outgrown simple "our way or the highway"
dismissals, IMO.
Secondly, I think the tristate option is the best suggestion I've seen
to date, both in terms of coverage and future flexibility (if needed).
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ben Reser <be...@reser.org>.
On Tue, Jul 9, 2013 at 1:25 AM, Stefan Sperling <st...@elego.de> wrote:
> On Tue, Jul 09, 2013 at 12:45:49AM -0700, Ben Reser wrote:
>> The option doesn't seem like a big barrier when you're applying it to
>> a handful of machines for yourself. Scale that effort up to a user
>> base of 20,000 users and the option stops looking like a reasonable
>> response. Resolving the proxy may not be possible in those cases as
>> well because the developers and the people running the Subversion
>> servers may have absolutely no control over the proxies. They may be
>> proprietary and not even have a way to resolve this. If you're a
>> large enterprise this solution just flat out doesn't help you much.
>
> I think this is a very important point.
>
> How many people use auto-props? Not many. Still, having auto-props
> configured at the client side made configuring this feature correctly
> in large deployments very inconvenient. Until 1.8 it requried central
> control over every client configuration. This was a concern in several
> companies I've visited.
>
> How many people are using proxies that don't support chunked requests?
> Probably not many. But the same argument applies.
>
>> I agree that chunked requests are very good for us. But I want to do
>> it in a way that is maximally compatible without making our users jump
>> through hoops.
>
> +1
>
> With so many other performance issues being fixed all the time,
> I don't think we should be worried about one extra request.
> We should be able to get the time spent on this extra request back
> somewhere else.
>
> 'svn merge' in 1.8 opens more connections that 1.7 did, and that is
> a performance hit but it didn't prevent the 1.8 release either.
So we had a conversation on IRC this morning about solutions.
This is what several of us seemed to agree was a reasonable compromise:
Have a tri-state option called http-chunked-requests. It would have
three states:
auto = Run the extra OPTIONS request and detect if chunked requests is
supported or not.
yes = Assume that HTTP/1.1 servers have chunked requests support (our
1.8.0 default behavior) and always use it against HTTP/1.1 servers.
no = Never use chunked requests.
With the default being auto.
This provides us a lot of flexibility in handling this. With a
tri-state we can reasonably change the default behavior if the proxy
environment changes or we find the auto detection to be too costly in
the wild. If we find a better way of automatically handling this we
can change what auto does.
This avoids the need for users to set an option in order to
interoperate, but users that want maximum performance can set it to
yes (assuming they don't have such a proxy in line).
Thoughts?
Re: svn commit: r1495419 - in
/subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c
util.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Jul 09, 2013 at 12:45:49AM -0700, Ben Reser wrote:
> The option doesn't seem like a big barrier when you're applying it to
> a handful of machines for yourself. Scale that effort up to a user
> base of 20,000 users and the option stops looking like a reasonable
> response. Resolving the proxy may not be possible in those cases as
> well because the developers and the people running the Subversion
> servers may have absolutely no control over the proxies. They may be
> proprietary and not even have a way to resolve this. If you're a
> large enterprise this solution just flat out doesn't help you much.
I think this is a very important point.
How many people use auto-props? Not many. Still, having auto-props
configured at the client side made configuring this feature correctly
in large deployments very inconvenient. Until 1.8 it requried central
control over every client configuration. This was a concern in several
companies I've visited.
How many people are using proxies that don't support chunked requests?
Probably not many. But the same argument applies.
> I agree that chunked requests are very good for us. But I want to do
> it in a way that is maximally compatible without making our users jump
> through hoops.
+1
With so many other performance issues being fixed all the time,
I don't think we should be worried about one extra request.
We should be able to get the time spent on this extra request back
somewhere else.
'svn merge' in 1.8 opens more connections that 1.7 did, and that is
a performance hit but it didn't prevent the 1.8 release either.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Tue, Jun 25, 2013 at 7:15 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 25.06.2013 15:37, Ivan Zhakov wrote:
>> On Tue, Jun 25, 2013 at 4:15 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>> On Sun, Jun 23, 2013 at 3:43 AM, Greg Stein <gs...@gmail.com> wrote:
>>>> On Fri, Jun 21, 2013 at 11:36 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>>> ...
>>>>> The fix seems to be pretty simple and could be safely backported (see
>>>>> patch). The only problem that probably we cannot backport addition of
>>>>> new configuration option due our backward compatibility policy. But
>>>>> may be I'm wrong.
>>>> The SVN_CONFIG_OPTION_HTTP_FORCE_HTTP10 symbol cannot be added.
>>>> Anything using and compiled against that could not be compiled against
>>>> 1.8.0.
>>>>
>>>> But: we don't have to add the public symbol. We can certainly read/use
>>>> the config option in a 1.8.1 release.
>>>>
>>>> But as I said else-thread, I think we should only disable chunked
>>>> requests. Not force HTTP/1.0 completely. And call it busted-proxy :-)
>>>>
>>> I agree that force-http10 is not good name and semantic. Actually
>>> these proxies is not busted: it's allowed to HTTP/1.1 proxies to
>>> require content-length if they want. And strictly speaking proxies may
>>> have different behavior for different requests.
>>>
>>> So I'm going to commit my patch with 'http-disable-chunked-requests'
>>> and nominate for backport. This should provide workaround for current
>>> users. And then continue discussion about full/automatic solution.
>>>
>> Added 'htttp-chunked-requests' configuration in r1496470.
>
> I'm really not a fan of this config knob. Anyone who carries their
> laptop around will effectively have to set this as the default, because
> you never know when the next weird proxy will pop up in front of your
> server. And disabling chunked requests by default is a lot worse than
> the extra non-pipelined request for broken proxies, IMO.
>
These proxies actually are not broken. Such behavior allowed by RFC.
Btw we already have knob to disabling compression or control maximum
number of concurrent connections to server.
Anyway I consider this configuration option as workaround, because any
full solution other than disable chunked requests by default will
require significant effort and time.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Wed, Jun 26, 2013 at 1:37 AM, Lieven Govaerts <sv...@mobsol.be> wrote:
> On Tue, Jun 25, 2013 at 5:15 PM, Branko Čibej <br...@wandisco.com> wrote:
>> On 25.06.2013 15:37, Ivan Zhakov wrote:
>>> On Tue, Jun 25, 2013 at 4:15 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>> On Sun, Jun 23, 2013 at 3:43 AM, Greg Stein <gs...@gmail.com> wrote:
>>>>> On Fri, Jun 21, 2013 at 11:36 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>>>> ...
>>>>>> The fix seems to be pretty simple and could be safely backported (see
>>>>>> patch). The only problem that probably we cannot backport addition of
>>>>>> new configuration option due our backward compatibility policy. But
>>>>>> may be I'm wrong.
>>>>> The SVN_CONFIG_OPTION_HTTP_FORCE_HTTP10 symbol cannot be added.
>>>>> Anything using and compiled against that could not be compiled against
>>>>> 1.8.0.
>>>>>
>>>>> But: we don't have to add the public symbol. We can certainly read/use
>>>>> the config option in a 1.8.1 release.
>>>>>
>>>>> But as I said else-thread, I think we should only disable chunked
>>>>> requests. Not force HTTP/1.0 completely. And call it busted-proxy :-)
>>>>>
>>>> I agree that force-http10 is not good name and semantic. Actually
>>>> these proxies is not busted: it's allowed to HTTP/1.1 proxies to
>>>> require content-length if they want. And strictly speaking proxies may
>>>> have different behavior for different requests.
>>>>
>>>> So I'm going to commit my patch with 'http-disable-chunked-requests'
>>>> and nominate for backport. This should provide workaround for current
>>>> users. And then continue discussion about full/automatic solution.
>>>>
>>> Added 'htttp-chunked-requests' configuration in r1496470.
>>
>> I'm really not a fan of this config knob. Anyone who carries their
>> laptop around will effectively have to set this as the default, because
>> you never know when the next weird proxy will pop up in front of your
>> server. And disabling chunked requests by default is a lot worse than
>> the extra non-pipelined request for broken proxies, IMO.
>
> Use https to connect to your repositories, than this whole issue does
> not impact you when on the road.
>
Good point: most forward proxies doesn't allow HTTP methods other than
GET/POST in default configuration. So users most likely already using
https when working through proxy. The issue is reverse proxies.
--
Ivan Zhakov
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Lieven Govaerts <sv...@mobsol.be>.
On Tue, Jun 25, 2013 at 5:15 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 25.06.2013 15:37, Ivan Zhakov wrote:
>> On Tue, Jun 25, 2013 at 4:15 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>> On Sun, Jun 23, 2013 at 3:43 AM, Greg Stein <gs...@gmail.com> wrote:
>>>> On Fri, Jun 21, 2013 at 11:36 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>>> ...
>>>>> The fix seems to be pretty simple and could be safely backported (see
>>>>> patch). The only problem that probably we cannot backport addition of
>>>>> new configuration option due our backward compatibility policy. But
>>>>> may be I'm wrong.
>>>> The SVN_CONFIG_OPTION_HTTP_FORCE_HTTP10 symbol cannot be added.
>>>> Anything using and compiled against that could not be compiled against
>>>> 1.8.0.
>>>>
>>>> But: we don't have to add the public symbol. We can certainly read/use
>>>> the config option in a 1.8.1 release.
>>>>
>>>> But as I said else-thread, I think we should only disable chunked
>>>> requests. Not force HTTP/1.0 completely. And call it busted-proxy :-)
>>>>
>>> I agree that force-http10 is not good name and semantic. Actually
>>> these proxies is not busted: it's allowed to HTTP/1.1 proxies to
>>> require content-length if they want. And strictly speaking proxies may
>>> have different behavior for different requests.
>>>
>>> So I'm going to commit my patch with 'http-disable-chunked-requests'
>>> and nominate for backport. This should provide workaround for current
>>> users. And then continue discussion about full/automatic solution.
>>>
>> Added 'htttp-chunked-requests' configuration in r1496470.
>
> I'm really not a fan of this config knob. Anyone who carries their
> laptop around will effectively have to set this as the default, because
> you never know when the next weird proxy will pop up in front of your
> server. And disabling chunked requests by default is a lot worse than
> the extra non-pipelined request for broken proxies, IMO.
Use https to connect to your repositories, than this whole issue does
not impact you when on the road.
> -- Brane
>
> --
> Branko Čibej | Director of Subversion
> WANdisco // Non-Stop Data
> e. brane@wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 25.06.2013 20:45, Greg Stein wrote:
> To that end, I find r1496470 to be insufficient. We need the dynamic
> stuff, and we need to fix the configuration option name. I'm happy to
> work on these two pieces, if we have a bit of consensus.
We (that is, you and I) certainly do. Is that a quorum? :)
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Jun 25, 2013 at 11:03 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Tue, Jun 25, 2013 at 10:45 PM, Greg Stein <gs...@gmail.com> wrote:
>> On Tue, Jun 25, 2013 at 11:55 AM, Philip Martin
>> <ph...@wandisco.com> wrote:
>>> Branko Čibej <br...@wandisco.com> writes:
>>>
>>>> I'm really not a fan of this config knob. Anyone who carries their
>>>> laptop around will effectively have to set this as the default, because
>>>> you never know when the next weird proxy will pop up in front of your
>>>> server. And disabling chunked requests by default is a lot worse than
>>>> the extra non-pipelined request for broken proxies, IMO.
>>
>> Right.
>>
>> Though I suspect most of the problems are reverse proxies in front of
>> a particular server, so you can put the config option into a [server]
>> config block instead of global. That will help to limit the problem,
>> but lack of dynamic detection is still a problem.
>>
> What is the benefit of dynamic detection enabled by some knob in config file?
The dynamic detection has a cost (1 extra request per connection),
that you might want to avoid by default (most environments won't need
the dynamic detection (especially corporate environments)). Only
enable the dynamic detection if you know the proxy has a problem with
chunkness, or if you're not sure it will stay that way, or ...
(not interfering with the rest of the discussion right now :-)
--
Johan
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Tue, Jun 25, 2013 at 10:45 PM, Greg Stein <gs...@gmail.com> wrote:
> On Tue, Jun 25, 2013 at 11:55 AM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Branko Čibej <br...@wandisco.com> writes:
>>
>>> I'm really not a fan of this config knob. Anyone who carries their
>>> laptop around will effectively have to set this as the default, because
>>> you never know when the next weird proxy will pop up in front of your
>>> server. And disabling chunked requests by default is a lot worse than
>>> the extra non-pipelined request for broken proxies, IMO.
>
> Right.
>
> Though I suspect most of the problems are reverse proxies in front of
> a particular server, so you can put the config option into a [server]
> config block instead of global. That will help to limit the problem,
> but lack of dynamic detection is still a problem.
>
What is the benefit of dynamic detection enabled by some knob in config file?
>> I suppose there might be a proxy that accepts some chunked requests but
>> not others and so we might get a 411 in the middle of a pipeline. For
>
> I doubt you would ever get a 411 in the *middle* of a series of
> pipelined request. The proxy is going to allow all, or allow none of
> the requests to use chunking.
>
> (or more precisely: I believe the code should make that assumption
> until we find/learn otherwise)
>
>> that case we probably need the config knob even if we add some automatic
>> detection. But that's a theoretical problem, the bug reports so far
>> involve a proxy with no support for chunked requests for which automatic
>> detection should be possible.
>
> Right. I believe the config knob should be "busted-proxy" and it
> should kick off a second OPTIONS request to detect whether chunked
> requests are allowed.
>
> To that end, I find r1496470 to be insufficient. We need the dynamic
> stuff, and we need to fix the configuration option name. I'm happy to
> work on these two pieces, if we have a bit of consensus.
>
I agree that option name can be improved. Just change it if you'd like.
Concerning dynamic stuff: I think it's better to add get_length()
method to serf_bucket and always send Content-Length. This solve all
potential issues with almost zero cost. In ra_serf we use
aggregate+simple buckets for XML bodies and file bucket for commit
(PUT) requests.
But this is not what can be implemented in svn 1.8.1 IMHO. That's why
I made very simple fix.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Lieven Govaerts <lg...@mobsol.be>.
On Tue, Jun 25, 2013 at 8:45 PM, Greg Stein <gs...@gmail.com> wrote:
> On Tue, Jun 25, 2013 at 11:55 AM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Branko Čibej <br...@wandisco.com> writes:
>>
>>> I'm really not a fan of this config knob. Anyone who carries their
>>> laptop around will effectively have to set this as the default, because
>>> you never know when the next weird proxy will pop up in front of your
>>> server. And disabling chunked requests by default is a lot worse than
>>> the extra non-pipelined request for broken proxies, IMO.
>
> Right.
>
> Though I suspect most of the problems are reverse proxies in front of
> a particular server, so you can put the config option into a [server]
> config block instead of global. That will help to limit the problem,
> but lack of dynamic detection is still a problem.
>
>> I suppose there might be a proxy that accepts some chunked requests but
>> not others and so we might get a 411 in the middle of a pipeline. For
>
> I doubt you would ever get a 411 in the *middle* of a series of
> pipelined request. The proxy is going to allow all, or allow none of
> the requests to use chunking.
>
> (or more precisely: I believe the code should make that assumption
> until we find/learn otherwise)
>
>> that case we probably need the config knob even if we add some automatic
>> detection. But that's a theoretical problem, the bug reports so far
>> involve a proxy with no support for chunked requests for which automatic
>> detection should be possible.
>
> Right. I believe the config knob should be "busted-proxy" and it
> should kick off a second OPTIONS request to detect whether chunked
> requests are allowed.
>
> To that end, I find r1496470 to be insufficient. We need the dynamic
> stuff, and we need to fix the configuration option name. I'm happy to
> work on these two pieces, if we have a bit of consensus.
I read Ivan's response earlier as saying we need a quick fix for 1.8.1
(the config knob) which gives us some time to implement a better
solution.
That better solution would be the extra request, is that something you
can reasonably implement in the coming days and all of us can test
sufficiently?
> Cheers,
> -g
Lieven
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jun 25, 2013 at 11:55 AM, Philip Martin
<ph...@wandisco.com> wrote:
> Branko Čibej <br...@wandisco.com> writes:
>
>> I'm really not a fan of this config knob. Anyone who carries their
>> laptop around will effectively have to set this as the default, because
>> you never know when the next weird proxy will pop up in front of your
>> server. And disabling chunked requests by default is a lot worse than
>> the extra non-pipelined request for broken proxies, IMO.
Right.
Though I suspect most of the problems are reverse proxies in front of
a particular server, so you can put the config option into a [server]
config block instead of global. That will help to limit the problem,
but lack of dynamic detection is still a problem.
> I suppose there might be a proxy that accepts some chunked requests but
> not others and so we might get a 411 in the middle of a pipeline. For
I doubt you would ever get a 411 in the *middle* of a series of
pipelined request. The proxy is going to allow all, or allow none of
the requests to use chunking.
(or more precisely: I believe the code should make that assumption
until we find/learn otherwise)
> that case we probably need the config knob even if we add some automatic
> detection. But that's a theoretical problem, the bug reports so far
> involve a proxy with no support for chunked requests for which automatic
> detection should be possible.
Right. I believe the config knob should be "busted-proxy" and it
should kick off a second OPTIONS request to detect whether chunked
requests are allowed.
To that end, I find r1496470 to be insufficient. We need the dynamic
stuff, and we need to fix the configuration option name. I'm happy to
work on these two pieces, if we have a bit of consensus.
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
> I'm really not a fan of this config knob. Anyone who carries their
> laptop around will effectively have to set this as the default, because
> you never know when the next weird proxy will pop up in front of your
> server. And disabling chunked requests by default is a lot worse than
> the extra non-pipelined request for broken proxies, IMO.
I suppose there might be a proxy that accepts some chunked requests but
not others and so we might get a 411 in the middle of a pipeline. For
that case we probably need the config knob even if we add some automatic
detection. But that's a theoretical problem, the bug reports so far
involve a proxy with no support for chunked requests for which automatic
detection should be possible.
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 25.06.2013 15:37, Ivan Zhakov wrote:
> On Tue, Jun 25, 2013 at 4:15 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On Sun, Jun 23, 2013 at 3:43 AM, Greg Stein <gs...@gmail.com> wrote:
>>> On Fri, Jun 21, 2013 at 11:36 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>> ...
>>>> The fix seems to be pretty simple and could be safely backported (see
>>>> patch). The only problem that probably we cannot backport addition of
>>>> new configuration option due our backward compatibility policy. But
>>>> may be I'm wrong.
>>> The SVN_CONFIG_OPTION_HTTP_FORCE_HTTP10 symbol cannot be added.
>>> Anything using and compiled against that could not be compiled against
>>> 1.8.0.
>>>
>>> But: we don't have to add the public symbol. We can certainly read/use
>>> the config option in a 1.8.1 release.
>>>
>>> But as I said else-thread, I think we should only disable chunked
>>> requests. Not force HTTP/1.0 completely. And call it busted-proxy :-)
>>>
>> I agree that force-http10 is not good name and semantic. Actually
>> these proxies is not busted: it's allowed to HTTP/1.1 proxies to
>> require content-length if they want. And strictly speaking proxies may
>> have different behavior for different requests.
>>
>> So I'm going to commit my patch with 'http-disable-chunked-requests'
>> and nominate for backport. This should provide workaround for current
>> users. And then continue discussion about full/automatic solution.
>>
> Added 'htttp-chunked-requests' configuration in r1496470.
I'm really not a fan of this config knob. Anyone who carries their
laptop around will effectively have to set this as the default, because
you never know when the next weird proxy will pop up in front of your
server. And disabling chunked requests by default is a lot worse than
the extra non-pipelined request for broken proxies, IMO.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Tue, Jun 25, 2013 at 4:15 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Sun, Jun 23, 2013 at 3:43 AM, Greg Stein <gs...@gmail.com> wrote:
>> On Fri, Jun 21, 2013 at 11:36 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>...
>>> The fix seems to be pretty simple and could be safely backported (see
>>> patch). The only problem that probably we cannot backport addition of
>>> new configuration option due our backward compatibility policy. But
>>> may be I'm wrong.
>>
>> The SVN_CONFIG_OPTION_HTTP_FORCE_HTTP10 symbol cannot be added.
>> Anything using and compiled against that could not be compiled against
>> 1.8.0.
>>
>> But: we don't have to add the public symbol. We can certainly read/use
>> the config option in a 1.8.1 release.
>>
>> But as I said else-thread, I think we should only disable chunked
>> requests. Not force HTTP/1.0 completely. And call it busted-proxy :-)
>>
> I agree that force-http10 is not good name and semantic. Actually
> these proxies is not busted: it's allowed to HTTP/1.1 proxies to
> require content-length if they want. And strictly speaking proxies may
> have different behavior for different requests.
>
> So I'm going to commit my patch with 'http-disable-chunked-requests'
> and nominate for backport. This should provide workaround for current
> users. And then continue discussion about full/automatic solution.
>
Added 'htttp-chunked-requests' configuration in r1496470.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Jul 5, 2013 at 11:00 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>...
> Serf-trunk now has serf_bucket_get_remaining() to get length of
> request body since r2008. Attached patch enables this functionality in
> ra_serf. Within this patch Content-Length will be send for every
> request for no cost and should make proxies (and servers) happy.
To be clear: it will use C-L given the current set of buckets used in
ra_serf. Not all buckets can determine their length, and if one of
those ever gets used ... a chunked request will occur.
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sun, Jun 30, 2013 at 7:37 PM, <km...@rockwellcollins.com> wrote:
>> On Tue, Jun 25, 2013 at 3:11 PM, <km...@rockwellcollins.com> wrote:
>> > > I agree that force-http10 is not good name and semantic. Actually
>> > > these proxies is not busted: it's allowed to HTTP/1.1 proxies to
>> > > require content-length if they want. And strictly speaking proxies may
>> > > have different behavior for different requests.
>> >
>> > From *our* standpoint, they are busted. Subversion wants to use
>> > chunked requests. If they don't support it, then they are busted.
>> > Simple as that.
>> >
>> > And we want to use a provocative name so that people understand
>> > something needs to be *fixed*. Fixed for us because we view them as
>> > *busted*.
>
>> From the *users* standpoint subversion is busted. Something that
>>
>> I'm not seeing the point. Subversion will (now) work, but we still
>> view the proxy as busted. It doesn't provide the performance
>> characteristics that Subversion wants and expects. Note that
>> Subversion is built to work against mod_dav_svn which is HTTP/1.1
>> with chunked requests. The interposition of a proxy that disables
>> chunked requests... busted.
>
> Yes you missed my point. Users don't care why something is
> broken, just that it is and that they now have to perform some
> manual operation to get it to work again. Score a -1 for svn user
> happiness. No reason to punish an user for something that is
> most likely outside of their control.
>
>> worked fine in 1.7 does not work in 1.8 without modifying potentially
>> unrelated things that neither the server admin or the client
>> user may have control over. (Think proxy at a hotel, etc.)
>>
>> Of course. But we can fix this. (and I believe, have fixed it)
>>
>> The spec states that 411 is an allowed response and is it also states
>> the client should prefer to not use chunked requests if possible.
>> Serf is being overly optimistic here.
>>
>> "Prefer" is not the same as "must" :-)
>
> True, but it is there for the exact reason we are arguing about :)
> (That clients which ignore this advice will not work correctly
> in a lot of real-world situations.)
>
>> In our current model, we prefer chunked. But there is a pretty
>> straightforward extension to serf's bucket model that should allow
>> us to use C-L in many situations. We *might* be able to do that in a
>> serf 1.x, but I'm not sure. Worst case, we'll add the feature in serf 2.x.
>
> I completely agree this is the preferred solution.
>
Serf-trunk now has serf_bucket_get_remaining() to get length of
request body since r2008. Attached patch enables this functionality in
ra_serf. Within this patch Content-Length will be send for every
request for no cost and should make proxies (and servers) happy.
--
Ivan Zhakov
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c
ra_serf.h serf.c util.c
Posted by km...@rockwellcollins.com.
> On Tue, Jun 25, 2013 at 3:11 PM, <km...@rockwellcollins.com> wrote:
> > > I agree that force-http10 is not good name and semantic. Actually
> > > these proxies is not busted: it's allowed to HTTP/1.1 proxies to
> > > require content-length if they want. And strictly speaking proxies
may
> > > have different behavior for different requests.
> >
> > From *our* standpoint, they are busted. Subversion wants to use
> > chunked requests. If they don't support it, then they are busted.
> > Simple as that.
> >
> > And we want to use a provocative name so that people understand
> > something needs to be *fixed*. Fixed for us because we view them as
> > *busted*.
> From the *users* standpoint subversion is busted. Something that
>
> I'm not seeing the point. Subversion will (now) work, but we still
> view the proxy as busted. It doesn't provide the performance
> characteristics that Subversion wants and expects. Note that
> Subversion is built to work against mod_dav_svn which is HTTP/1.1
> with chunked requests. The interposition of a proxy that disables
> chunked requests... busted.
Yes you missed my point. Users don't care why something is
broken, just that it is and that they now have to perform some
manual operation to get it to work again. Score a -1 for svn user
happiness. No reason to punish an user for something that is
most likely outside of their control.
> worked fine in 1.7 does not work in 1.8 without modifying potentially
> unrelated things that neither the server admin or the client
> user may have control over. (Think proxy at a hotel, etc.)
>
> Of course. But we can fix this. (and I believe, have fixed it)
>
> The spec states that 411 is an allowed response and is it also states
> the client should prefer to not use chunked requests if possible.
> Serf is being overly optimistic here.
>
> "Prefer" is not the same as "must" :-)
True, but it is there for the exact reason we are arguing about :)
(That clients which ignore this advice will not work correctly
in a lot of real-world situations.)
> In our current model, we prefer chunked. But there is a pretty
> straightforward extension to serf's bucket model that should allow
> us to use C-L in many situations. We *might* be able to do that in a
> serf 1.x, but I'm not sure. Worst case, we'll add the feature in serf
2.x.
I completely agree this is the preferred solution.
(Sorry for being so persistent about this. Just a pretty big sore
spot at the moment because I've been implementing a subversion aware
proxy. To be honest neon has been very easy to work with. Serf
has been nothing but problematic. Old Serf versions were very
easy to crash and get into infinite loops especially when dealing with
proxy authentication. I feel bad I didn't have time to look into
the problems until recently, especially since you are usually so quick
to fix them - all problems I found were already fixed in later Serf
versions than I was testing. I look forward to the latest 1.2.2 Serf
changes since I think it should finally be stable enough to be supported
in my expected non-utopia situations.)
Kevin R.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ben Reser <be...@reser.org>.
On Thu, Jul 11, 2013 at 10:48 AM, Branko Čibej <br...@wandisco.com> wrote:
> I suspect that's completely irrelevant, since the probe issues OPTIONS
> requests which are very small and should never spill to disk. The big
> requests would be GET/PUT of large files and/or deltas, IIRC -- but
> those shouldn't have to be retried if the probe does the right thing.
I think you misunderstood the purpose of the test. The tests I did
won't show any significant change from the extra OPTIONS request (I
could have just did no and yes but I did auto for completeness)
because the latency is so low (<1ms) and as you point out the
spillbuff doesn't matter for OPTIONS. Greg is concerned that people
may set the tristate to no and then eat performance loses from not
using chunking when they could be. So the point here is to show what
that impact would be, which based on the tests I did is minimal.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 11.07.2013 19:42, Ben Reser wrote:
> Now part of the concern over CL is that we'd end up having to use temp
> files. My test setup was using ramdisk for the test workspace and the
> system disk is a SSD, so that might help hide some of that. Even more
> importantly I believe the request needs to be over 100k before it'd
> need a tempfile, so I kinda doubt our test suite exercises that much.
I suspect that's completely irrelevant, since the probe issues OPTIONS
requests which are very small and should never spill to disk. The big
requests would be GET/PUT of large files and/or deltas, IIRC -- but
those shouldn't have to be retried if the probe does the right thing.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Lieven Govaerts <lg...@apache.org>.
On Wed, Jul 10, 2013 at 10:41 AM, Branko Čibej <br...@wandisco.com> wrote:
> On 10.07.2013 10:20, Branko Čibej wrote:
>> On 10.07.2013 05:43, Greg Stein wrote:
>>> On Tue, Jul 9, 2013 at 10:50 PM, Branko Čibej <br...@wandisco.com> wrote:
>>>> On 09.07.2013 05:53, Greg Stein wrote:
>>>> ...
>>>>> For *this* project, that is absolutely the case. We have never said
>>>>> "let's work against any server anybody decides to implement." We write
>>>>> to our client, and our server, and third parties adapt to our changes.
>>>> You can't seriously claim that and in the same breath talk about HTTP
>>>> transport. We're either compliant or not. If we're not compliant, we
>>>> either fix the bug or stop calling it HTTP. You cannot unilaterally
>>>> decide that the HTTP spec says something else than is actually written
>>>> there (and corroborated by later, albeit draft versions).
>>> Huh? We're compliant. Why aren't we?
>> Taking the "the client MAY ..." literally, we're compliant ... but
>> there's something to be said about about following the spirit of the
>> spec as well as the letter.
>>
>> I wouldn't really mind breaking peoples' setups if Subversion had issued
>> chunked requests before, or if we'd announced well in advance that not
>> supporting chunked requests is going to break something -- the way we
>> did, for example, warn about the server-side effects of HTTPv2 and its
>> multiple connections and many more requests. But it did not cross our minds.
>>
>> We'd always sold ra_serf as "ra_neon but better". So now we're faced
>> with a regression that we can fix in one of two ways: lay the burden on
>> each and every user that uses a site that's behind a certain kind of
>> compliant proxy, or lay it on site administrators. I maintain that the
>> latter is the better solution; even if it means making opening the
>> session less than optimal until some 1.8.2/serf-1.4.0 combination gets
>> released.
>
> And note that we can amortize the 'less optimal' by backporting Ivan's
> patch with the RA session pool in libsvn_client. Though I'm not
> suggesting we do that in 1.8.1.
That patch doesn't work yet, but it's a good example of the many other
optimisations we can still do in ra_serf performance.
(Also) Ivan's work on reusing SSL sessions between connections in the
same SSL context should also help a lot, but that's for https only.
Lieven
> -- Brane
>
>
> --
> Branko Čibej | Director of Subversion
> WANdisco // Non-Stop Data
> e. brane@wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 10.07.2013 10:20, Branko Čibej wrote:
> On 10.07.2013 05:43, Greg Stein wrote:
>> On Tue, Jul 9, 2013 at 10:50 PM, Branko Čibej <br...@wandisco.com> wrote:
>>> On 09.07.2013 05:53, Greg Stein wrote:
>>> ...
>>>> For *this* project, that is absolutely the case. We have never said
>>>> "let's work against any server anybody decides to implement." We write
>>>> to our client, and our server, and third parties adapt to our changes.
>>> You can't seriously claim that and in the same breath talk about HTTP
>>> transport. We're either compliant or not. If we're not compliant, we
>>> either fix the bug or stop calling it HTTP. You cannot unilaterally
>>> decide that the HTTP spec says something else than is actually written
>>> there (and corroborated by later, albeit draft versions).
>> Huh? We're compliant. Why aren't we?
> Taking the "the client MAY ..." literally, we're compliant ... but
> there's something to be said about about following the spirit of the
> spec as well as the letter.
>
> I wouldn't really mind breaking peoples' setups if Subversion had issued
> chunked requests before, or if we'd announced well in advance that not
> supporting chunked requests is going to break something -- the way we
> did, for example, warn about the server-side effects of HTTPv2 and its
> multiple connections and many more requests. But it did not cross our minds.
>
> We'd always sold ra_serf as "ra_neon but better". So now we're faced
> with a regression that we can fix in one of two ways: lay the burden on
> each and every user that uses a site that's behind a certain kind of
> compliant proxy, or lay it on site administrators. I maintain that the
> latter is the better solution; even if it means making opening the
> session less than optimal until some 1.8.2/serf-1.4.0 combination gets
> released.
And note that we can amortize the 'less optimal' by backporting Ivan's
patch with the RA session pool in libsvn_client. Though I'm not
suggesting we do that in 1.8.1.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ben Reser <be...@reser.org>.
On Wed, Jul 10, 2013 at 3:57 PM, Ben Reser <be...@reser.org> wrote:
> LOG --diff: RTT*(1+(revisions*2) [It appears we make 2 connections
> per revision to do the diff, ouch, I guess that explains why log
> --diff is so slow already]
Err I mean we make 2 sessions.
For log --diff with make 1+(revisions*2) sessions and 2+(revisions*4)
connections i.e. we make 2 connections for every session.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ben Reser <be...@reser.org>.
On Wed, Jul 10, 2013 at 9:57 PM, Greg Stein <gs...@gmail.com> wrote:
> On Wed, Jul 10, 2013 at 6:57 PM, Ben Reser <be...@reser.org> wrote:
>> I have about 160ms of latency to this server. Mostly because I'm on
>> the west coast of the US and it's in the UK someplace based on it's
>> domain name.
>
> Human perception is right around the 250ms mark. User interface
> designers use that metric for response time.
[...]
> In any case. In your example, with a 160ms latency, then RTT*2 is past
> that barrier. The user will notice a lag.
>
> That said: there may already be other lag, so RTT*2 might be 320ms of
> a 5s response time.
I'd say that there absolutely is other lag in all of these cases. If
we were always responding under the 250ms mark that you point out
above (and that I agree with), then I'd be a lot more concerned about
this change. However, every one of these cases results in a lot more
delay that that. I guess I could have measured that and measured how
long the delay was for the first result. But I'm pretty confident in
saying this change doesn't push us past a 250ms delay we're not
already past.
Consider that the log --diff example I gave. It used 91 requests, 11
of which were extra OPTION request to detect chunking, in order to
provide the log and diff of 5 revisions. It used 11 sessions and 22
connections to do so. Even without this patch log --diff noticeably
pauses on each log entry to retrieve the diff, well past a 250ms delay
when I'm using it against svn.us.apache.org which I have around 20ms
latency to.
On another front I ran our test suite and timed it in several
configurations (this is with the tristate branch and its option
meanings):
auto(through nginx 1.2.9)
real 30m41.060s
user 15m14.803s
sys 12m34.307s
no(through nginx 1.2.9)
real 30m50.127s
user 15m16.974s
sys 12m37.019s
yes(direct)
real 30m23.280s
user 15m13.347s
sys 12m27.483s
auto(direct)
real 30m35.522s
user 15m15.198s
sys 12m29.660s
no(direct)
real 30m22.735s
user 15m14.443s
sys 12m30.666s
Obviously latency is almost a non-issue here because everything is
local (<1ms) so what I'm really looking to show is the relative
performance of CL vs Chunked requests.
I didn't repeat these tests, so you're seeing a single result. So
there may be some variation between test runs that isn't averaged out.
As you can see though there is very little difference in these test
as far as time. The biggest difference is 23 seconds, which is just
slightly more than 1% performance degrediction between yes(direct) and
no(through nginx 1.2.9) and at least some of that comes from nginx
being in line.
Now part of the concern over CL is that we'd end up having to use temp
files. My test setup was using ramdisk for the test workspace and the
system disk is a SSD, so that might help hide some of that. Even more
importantly I believe the request needs to be over 100k before it'd
need a tempfile, so I kinda doubt our test suite exercises that much.
But so far I'm not seeing any major performance penalty in using CL
instead of chunked requests. Obviously in the future that may become
more significant.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jul 10, 2013 at 6:57 PM, Ben Reser <be...@reser.org> wrote:
>...
> I have about 160ms of latency to this server. Mostly because I'm on
> the west coast of the US and it's in the UK someplace based on it's
> domain name.
Human perception is right around the 250ms mark. User interface
designers use that metric for response time.
[ Google shoots for some huge percentile of searches to come in under
that. Once you cross the 250ms mark with *no response*, then the user
perceives a delay. In Google's case, that delay means a user goes
elsewhere, and Google loses money. They've demonstrated a specific
correlation between response time and revenue. It's a bit frightening
when you file an SEC annual report that says "our revenue is tied to
our HTTP response time". (okay, they didn't file that, but they
*could*) ]
Of course, you can play certain tricks, like:
$ svn ls $URL
<... 150ms ...>
Listing of $URL:
<... 200ms ...>
$FILES
In any case. In your example, with a 160ms latency, then RTT*2 is past
that barrier. The user will notice a lag.
That said: there may already be other lag, so RTT*2 might be 320ms of
a 5s response time.
>...
Thanks for running these numbers. I can see that it might also be
constructive to add some kind of connection/request profiling into
ra_serf that we could use to direct optimizations in the future.
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ben Reser <be...@reser.org>.
On Wed, Jul 10, 2013 at 7:45 AM, Greg Stein <gs...@gmail.com> wrote:
> IOW, while I may agree with "regression", I'm not sure that we should
> impact 99+% in order to deal with it. I'm not sure it is a reasonable
> tradeoff.
Providing some real world numbers.
I tested against: http://svn.mkgmap.org.uk/mkgmap/trunk
Which is known to have a proxy in front of it that rejects chunked requests.
While I tested against a server that has the problem I don't expect
the difference between real world performance of those with or without
the issue to differ, I expect the response to the second OPTIONS
request to be about the same regardless. Doing it against a busted
server made it a little easier because I could just count the 411s.
I have about 160ms of latency to this server. Mostly because I'm on
the west coast of the US and it's in the UK someplace based on it's
domain name.
I didn't bother to figure out what version of SVN this server is
running, but that would impact this because of the various changes
we've made to our protocol over time.
I'll present the results here in RTT*number of times we made an extra
OPTIONS request.
CHECKOUT: RTT*2 [didn't bother to test export, assume it's the same as checkout]
LOG: RTT*1
LOG --diff: RTT*(1+(revisions*2) [It appears we make 2 connections
per revision to do the diff, ouch, I guess that explains why log
--diff is so slow already]
MERGE -c 2652 ^/branches/options2: RTT*2 [several files]
MERGE -c 2498 ^/branches/options2 RTT*2 [fewer files, I'm guessing
there's something that will make merge use more but I'm not sure what]
UPDATE: RTT*1 [didn't bother to test switch I'm assuming it's the same
as UPDATE]
BLAME: RTT*N [where N is the number of files being blamed, not that I
think people usually blame more than one file in a go]
LS ^/branches/options2: RTT*1
MERGEINFO ^/branches/options2: RTT*6 [no idea why this is so high I
didn't investigate]
STATUS -u: RTT*2
I obviously don't have write access to this server so I didn't test
anything that required write access.
Hopefully this gives everyone some real world info
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jul 10, 2013 at 4:20 AM, Branko Čibej <br...@wandisco.com> wrote:
>...
> Taking the "the client MAY ..." literally, we're compliant ... but
> there's something to be said about about following the spirit of the
> spec as well as the letter.
You're the one debating compliance.
> I wouldn't really mind breaking peoples' setups if Subversion had issued
> chunked requests before,
There's *always* a first time. We gotta start some time.
> or if we'd announced well in advance that not
> supporting chunked requests is going to break something -- the way we
> did, for example, warn about the server-side effects of HTTPv2 and its
> multiple connections and many more requests. But it did not cross our minds.
Same with the level of bustitude out there on the Internet. We had no
way to measure it until now. Breakage happens. Obviously, we try to
avoid it. *shrug*
> We'd always sold ra_serf as "ra_neon but better". So now we're faced
> with a regression that we can fix in one of two ways: lay the burden on
> each and every user that uses a site that's behind a certain kind of
> compliant proxy, or lay it on site administrators. I maintain that the
> latter is the better solution; even if it means making opening the
> session less than optimal until some 1.8.2/serf-1.4.0 combination gets
> released.
I already said that I could agree its a regression.
Is the community going to be just as up-in-arms about the "svn status
--xml" regression? Is that one just as important to fix? How do you
measure user impact and importance? Does this proxy problem rise to
the same level?
IOW, while I may agree with "regression", I'm not sure that we should
impact 99+% in order to deal with it. I'm not sure it is a reasonable
tradeoff.
The "auto" suggestion is good, but it *will* impact 1.8.x users. We
can solve most of that in the 1.9.x timeframe. But that still leaves
1.8.x out in the cold. CMike spent a lot of time on HTTPv2 reducing
the number of (synchronous) requests to the server. I hate to see us
retreating from his work, to try and solve something with a miniscule
impact, which can be solved with a client knob or a proxy upgrade. We
can use the knob/upgrade strategy for 1.8.x and handle it better in
1.9.x (and possibly deprecate it, if our detection/solution gets good
enough).
So that's my issue right now: is knob/upgrade good enough for 1.8.x,
with better answers in the 1.9.x timeframe?
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 10.07.2013 05:43, Greg Stein wrote:
> On Tue, Jul 9, 2013 at 10:50 PM, Branko Čibej <br...@wandisco.com> wrote:
>> On 09.07.2013 05:53, Greg Stein wrote:
>> ...
>>> For *this* project, that is absolutely the case. We have never said
>>> "let's work against any server anybody decides to implement." We write
>>> to our client, and our server, and third parties adapt to our changes.
>> You can't seriously claim that and in the same breath talk about HTTP
>> transport. We're either compliant or not. If we're not compliant, we
>> either fix the bug or stop calling it HTTP. You cannot unilaterally
>> decide that the HTTP spec says something else than is actually written
>> there (and corroborated by later, albeit draft versions).
> Huh? We're compliant. Why aren't we?
Taking the "the client MAY ..." literally, we're compliant ... but
there's something to be said about about following the spirit of the
spec as well as the letter.
I wouldn't really mind breaking peoples' setups if Subversion had issued
chunked requests before, or if we'd announced well in advance that not
supporting chunked requests is going to break something -- the way we
did, for example, warn about the server-side effects of HTTPv2 and its
multiple connections and many more requests. But it did not cross our minds.
We'd always sold ra_serf as "ra_neon but better". So now we're faced
with a regression that we can fix in one of two ways: lay the burden on
each and every user that uses a site that's behind a certain kind of
compliant proxy, or lay it on site administrators. I maintain that the
latter is the better solution; even if it means making opening the
session less than optimal until some 1.8.2/serf-1.4.0 combination gets
released.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jul 9, 2013 at 10:50 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 09.07.2013 05:53, Greg Stein wrote:
>...
>> For *this* project, that is absolutely the case. We have never said
>> "let's work against any server anybody decides to implement." We write
>> to our client, and our server, and third parties adapt to our changes.
>
> You can't seriously claim that and in the same breath talk about HTTP
> transport. We're either compliant or not. If we're not compliant, we
> either fix the bug or stop calling it HTTP. You cannot unilaterally
> decide that the HTTP spec says something else than is actually written
> there (and corroborated by later, albeit draft versions).
Huh? We're compliant. Why aren't we?
> Yes, we've come across broken proxies any number of times; but the
> refusal to accept chunked transport in HTTP/1.1 is clearly not broken
> since its anticipated and described by the HTTP spec.
>From our perspective, somebody broke our communication to mod_dav_svn.
Are they allowed to? Sure. They return a 411 like the spec says. And
following that spec, we choose not to resend with a C-L header.
The proxy is not inherently busted, per the spec. But it certainly
busted our communication.
> And it is definitely not OK to make our users collateral damage in a
> crusade against "busted" proxies. It's fine to print a big fat warning
I'm fine with the users argument. Never said otherwise.
I said, "I don't want the 99% to suffer at the hands of the edge
case." And especially because the edge case is usually solvable. (and
yes, I know; I said "usually")
I can agree this is a regression. We run into regressions in every
release. We don't even bother to solve all of them (ref: api-errata).
To that end, is "yeah. sorry. regression in 1.8. there is a knob to
fix it." an improper response?
I'm not really sure of the answer to that. But I think it's a fair question.
>...
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 09.07.2013 05:53, Greg Stein wrote:
> On Mon, Jul 8, 2013 at 9:52 PM, Ben Reser <ben@reser.org
> <ma...@reser.org>> wrote:
> >...
>
> Greg's argument here mostly depends on the idea that Subversion is
> built to work against mod_dav_svn and any proxy or implementation that
> doesn't implement HTTP/1.1 with chunked requests support as
> mod_dav_svn does is busted. I find this argument very weak and
> unconvincing. Especially since this assumes that mod_dav_svn is the
> only server side implementation of our protocol. We know this is not
>
>
> For *this* project, that is absolutely the case. We have never said
> "let's work against any server anybody decides to implement." We write
> to our client, and our server, and third parties adapt to our changes.
You can't seriously claim that and in the same breath talk about HTTP
transport. We're either compliant or not. If we're not compliant, we
either fix the bug or stop calling it HTTP. You cannot unilaterally
decide that the HTTP spec says something else than is actually written
there (and corroborated by later, albeit draft versions).
Yes, we've come across broken proxies any number of times; but the
refusal to accept chunked transport in HTTP/1.1 is clearly not broken
since its anticipated and described by the HTTP spec.
And it is definitely not OK to make our users collateral damage in a
crusade against "busted" proxies. It's fine to print a big fat warning
that some host is behind a proxy that doesn't support such-and-such a
feature of the HTTP protocol which will make their latency suck; they
can go complain to the site admins. It's not fine to tell them to fiddle
with the config files that worked just fine before the upgrade to 1.8.
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Jul 8, 2013 at 11:53 PM, Greg Stein <gs...@gmail.com> wrote:
>...
> So let's say you have a RTT of 500ms, that means your command is going
>>
> to be delayed by 4 seconds assuming 8 connections, it also means that
>> you're going to have a pretty bad time using Subversion anyway. Yes
>> that sucks, but I doubt very many of our commands actually make use of
>> all 8 of those connections, remember we only actually require 2.
>> Right now from the US I have a RTT to svn.eu.apache.org of about 180ms
>> and to svn.us.apache.org of about 20ms, meaning worst case scenario
>> for those is a delay of 1.4 seconds and 160ms.
>>
>
Data point: using a packet analyzer, the extra request (for me, to
svn.us.apache.org) adds 117ms.
Ping time is comparable (leading me to assume it's all network, rather than
processing-based lag).
Cheers,
-g
RE: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ben Reser [mailto:ben@reser.org]
> Sent: dinsdag 9 juli 2013 09:46
> To: Greg Stein
> Cc: kmradke; Subversion Development
> Subject: Re: svn commit: r1495419 - in
> /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c
util.c
>
> On Mon, Jul 8, 2013 at 8:53 PM, Greg Stein <gs...@gmail.com> wrote:
> > For *this* project, that is absolutely the case. We have never said
"let's
> > work against any server anybody decides to implement." We write to our
> > client, and our server, and third parties adapt to our changes.
>
> But we have said that a newer Subversion client will continue to work
> against an older server. The net effect of this issue is hardly
> different for users trapped behind these proxies from us requiring
> some new feature only provided in a 1.8 server to use a 1.8 client. I
> realize that's not entirely true since our server has always supported
> chunked requests, but I don't think users see this distinction. I
> think we owe our users to go the extra mile here.
Note that we haven't got a single report from a user with a proxy that
behaves like this.
All user reports on our lists (and to the AnkhSVN lists) are against servers
that are setup with a front-end-proxy to the internet.
Normal users wouldn't even see this as a proxy setup. I wouldn't be
surprised if most of these are already upgraded, as that is the most likely
reason why we don't get additional user reports on our lists.
Bert
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ben Reser <be...@reser.org>.
On Mon, Jul 8, 2013 at 8:53 PM, Greg Stein <gs...@gmail.com> wrote:
> For *this* project, that is absolutely the case. We have never said "let's
> work against any server anybody decides to implement." We write to our
> client, and our server, and third parties adapt to our changes.
But we have said that a newer Subversion client will continue to work
against an older server. The net effect of this issue is hardly
different for users trapped behind these proxies from us requiring
some new feature only provided in a 1.8 server to use a 1.8 client. I
realize that's not entirely true since our server has always supported
chunked requests, but I don't think users see this distinction. I
think we owe our users to go the extra mile here.
> Read that second sentence. A server (or proxy) sends a 411 at their own
> peril. A client is not obligated to retry with a C-L header.
I have a very hard time accepting that we're HTTP/1.1 compliant when
we just throw an error at a user when they're going through a proxy
that is HTTP/1.1 compliant but has chosen not to implement chunked
requests. Capabilities between the client and server should be
negotiated. I'd say that the RFC 2616 dropped the ball here. I
understand your position that essentially we're under no obligation to
try harder. But again I think we owe it to our users to do so.
> We detect the busted proxy at runtime, and adapt for it. However, the test
> is costly (RTT, as you note) and so we enable only upon request.
Yup and I believe we haven't done a good enough job here.
The option doesn't seem like a big barrier when you're applying it to
a handful of machines for yourself. Scale that effort up to a user
base of 20,000 users and the option stops looking like a reasonable
response. Resolving the proxy may not be possible in those cases as
well because the developers and the people running the Subversion
servers may have absolutely no control over the proxies. They may be
proprietary and not even have a way to resolve this. If you're a
large enterprise this solution just flat out doesn't help you much.
> Careful. A single ra_session only needs to perform the busted-proxy test
> once. All connections share the result of that test.
>
> Problems arise with multiple sessions (think: merging).
You can't assume the capabilities of one connection are equal to
another. For instance if WCCP is being used to direct clients through
a reverse proxy there is absolutely no guarantee that all the proxy
servers in the WCCP pool have equal capabilities.
You might say, that if someone does that then they're doing things
wrong. But I'd say that someone implementing a large proxy setup is
almost certainly going to upgrade their proxy servers at some point
and they're going to do it by taking one node down,
upgrading/replacing it and putting the new one back in the pool. So
that they can maintain continuous uptime and their capacity.
I know we're probably caching this on the ra session, but that's
probably wrong. In practice that's probably not going to cause
problems for very many people, but it's still wrong.
> Possibly. I hate telling people "you're slower because 1% of users choose
> not to get their busted proxy fixed."
So do I. But I want a fix that doesn't ask our users to jump through hoops.
> Chunked requests are *good* for us. We *want* chunked requests. It gives us
> flexibility in our design where C-L does not. We can generate requests where
> we don't know the length ahead of time. There are two main areas where that
> is very important:
>
> 1) custom request buckets that dynamically generate the request on demand
> (via callbacks into other parts of svn)
>
> 2) applying gzip and other encodings on the request
>
> Using C-L completely denies these capabilities.
I realize you're thinking long term here Greg, but I'm thinking short
term. We're not using these things right now. We don't need chunked
requests.
So let's solve our users immediate issues by turning off chunked requests.
Then let's figure out a performant way of detecting when we can use
chunked requests safely. If we can do that then people using these
proxies without chunked requests will probably end up with less
performance, but we can add a note to our release notes telling people
to look for this and make sure this isn't happening to get the best
performance.
I agree that chunked requests are very good for us. But I want to do
it in a way that is maximally compatible without making our users jump
through hoops. If Ivan's work on the Serf side isn't sufficient to
get us there then let's do what's needed to get us there.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
Heya ... I'm not going to repond point-by-point. Your email covers
everything quite well. I only have a couple issues, noted below:
On Mon, Jul 8, 2013 at 9:52 PM, Ben Reser <be...@reser.org> wrote:
>...
> Greg's argument here mostly depends on the idea that Subversion is
> built to work against mod_dav_svn and any proxy or implementation that
> doesn't implement HTTP/1.1 with chunked requests support as
> mod_dav_svn does is busted. I find this argument very weak and
> unconvincing. Especially since this assumes that mod_dav_svn is the
> only server side implementation of our protocol. We know this is not
>
For *this* project, that is absolutely the case. We have never said "let's
work against any server anybody decides to implement." We write to our
client, and our server, and third parties adapt to our changes.
We switched from entries to wc.db in WC-NG. Should we have worried that the
change would break third-parties that parsed entries? Nope.
Codeplex had some issues with our requests. Did we change our request to
get Codeplex working? Nope.
We cannot and choose not to worry about what third parties are doing. We
*do* try and keep our protocol reasonably documented, stable, and
understandable so they *can* work with us. But we've always chosen that to
be a one-way street (primarily; we have tweaked low-hanging fruit to help
third parties).
To that end, we are designed to work against mod_dav_svn. If something
interferes with our operation, then I say its *breaks* our
performance/correctness goals.
>...
> While I think it's been sufficiently proven elsewhere in this thread
> I'll make the argument that we are not HTTP/1.1 compliant if we don't
> handle this since part of my argument above assumes that we're not
> compliant if we don't handle this case.
>
Huh? RFC 2616 says:
10.4.12 411 Length Required
The server refuses to accept the request without a defined Content- Length.
The client MAY repeat the request if it adds a valid Content-Length header
field containing the length of the message-body in the request message.
Read that second sentence. A server (or proxy) sends a 411 at their own
peril. A client is not obligated to retry with a C-L header.
We detect the busted proxy at runtime, and adapt for it. However, the test
is costly (RTT, as you note) and so we enable only upon request.
>...
> The chair of the working group posted this:
> http://www.mnot.net/blog/2011/07/11/what_proxies_must_do
He says, " a good intermediary will pass through chunked requests such as
this one:" ... we expect chunked requests to work. When an intermediary
doesn't allow them... I call that interfering with our operation. It should
be fixed.
>...
> We limit RA serf to a max of 8 connections, but with things like
> svn:externals we open new connections and then close them, so 8 may
> not be the absolute worst case scenario.
> [[[
> #define SVN_RA_SERF__MAX_CONNECTIONS_LIMIT 8
> ]]]
>
Careful. A single ra_session only needs to perform the busted-proxy test
once. All connections share the result of that test.
Problems arise with multiple sessions (think: merging).
>
> We also require that we can make at least 2 connections to the server.
> So it's hard to say what the typical behavior would be across so many
> different configurations. But worst case scenario is always going to
> be number_connections_opened * round_trip_time (RTT).
>
number_RA_sessions_opened * RTT.
>
> The corporate users that Ivan is worried about that are likely to have
> no issues here because they have no such proxy in their way are the
> ones likely to have the lowest RTT.
>
> So let's say you have a RTT of 500ms, that means your command is going
> to be delayed by 4 seconds assuming 8 connections, it also means that
> you're going to have a pretty bad time using Subversion anyway. Yes
> that sucks, but I doubt very many of our commands actually make use of
> all 8 of those connections, remember we only actually require 2.
> Right now from the US I have a RTT to svn.eu.apache.org of about 180ms
> and to svn.us.apache.org of about 20ms, meaning worst case scenario
> for those is a delay of 1.4 seconds and 160ms.
>
> I have a very hard time believing that the above is worth the concern.
> We should implement this detection behavior by default.
>
Possibly. I hate telling people "you're slower because 1% of users choose
not to get their busted proxy fixed."
>
> Given Ivan's work on Serf to prefer sending Content-Length, we can
> disable it if we're built against a Serf that behaves this way.
>
> I think that we should defer from adding an option to manipulate this
> behavior until such a time that we find significant performance
> issues. Especially if down the line we're not going to need it
> because we can prefer CL.
>
I wish you guys would stop thinking that work is a panacea. That is is some
kind of grand "fix everything".
Chunked requests are *good* for us. We *want* chunked requests. It gives us
flexibility in our design where C-L does not. We can generate requests
where we don't know the length ahead of time. There are two main areas
where that is very important:
1) custom request buckets that dynamically generate the request on demand
(via callbacks into other parts of svn)
2) applying gzip and other encodings on the request
Using C-L completely denies these capabilities.
We target mod_dav_svn, and chunked requests are part of our arsenal for
making the communication performant.
>...
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ben Reser <be...@reser.org>.
First of all a bit of top posting here because I want to paint my
response to Greg and then ultimately Ivan's email with a bit of
background. In the process of preparing to do the 1.8.1 release I
spent quite a bit of time reviewing nominated changes. I'll admit I
hadn't paid too much attention to this issue while I was on vacation
or when I got back last week. It seemed like there was a agreed upon
solution. What I didn't realize is that we'd agreed upon an option
named "busted-proxy" and agreed upon forcing users to set an option in
order to work in these circumstances.
I'm replying to Greg's and Ivan's emails because after re-reading this
entire thread these two emails present the two essential arguments for
the option to exist at all.
1) That the proxies are busted and that having an option named
busted-proxy we will encourage proxy administrators to fix the
problem. Greg's email makes this argument.
2) That there is a performance cost of making the extra request and
that the request is too high of a cost for users that don't have these
proxies in their way. Ivan's email makes this argument.
Most people on this thread have argued that we should automatically
detect this behavior and adjust accordingly. The above are the two
strongest arguments for the option.
On Fri, Jun 28, 2013 at 8:37 PM, Greg Stein <gs...@gmail.com> wrote:
> I'm not seeing the point. Subversion will (now) work, but we still view the
> proxy as busted. It doesn't provide the performance characteristics that
> Subversion wants and expects. Note that Subversion is built to work against
> mod_dav_svn which is HTTP/1.1 with chunked requests. The interposition of a
> proxy that disables chunked requests... busted.
Greg's argument here mostly depends on the idea that Subversion is
built to work against mod_dav_svn and any proxy or implementation that
doesn't implement HTTP/1.1 with chunked requests support as
mod_dav_svn does is busted. I find this argument very weak and
unconvincing. Especially since this assumes that mod_dav_svn is the
only server side implementation of our protocol. We know this is not
the case. GitHub has their own implementation of the DAV server side
in order to support Subversion clients checking out of git repos
hosted there. I'm sure there are others out there we aren't
necessarily even aware of (I can think of at least one more but I'm
not going to go into the details since it's not important here).
If we fail to follow the standard those sorts of implementations are
the things we risk losing as a project. In this case I don't think
GitHub's functionality breaks because of this issue. But let's say
that their implementation for some reason chose not to support chunked
requests? If we started breaking GitHub's server implementation with
our clients on a regular basis because we wanted to require the use
features of HTTP that we have not required in the past what do you
suppose they would do? I'd expect that they'd probably eventually
give up on bothering to support our client.
I see this as a turning point for this project. If we go down this
road, then even our DAV protocol is not following a standard. I know
we threw out DeltaV with HTTPv2, but the distinction here is that we
were backwards compatible. In this case we are not backwards
compatible with what we supported in Subversion 1.7.x because we did
not fail when proxied through a proxy that refused chunked requests.
While I think it's been sufficiently proven elsewhere in this thread
I'll make the argument that we are not HTTP/1.1 compliant if we don't
handle this since part of my argument above assumes that we're not
compliant if we don't handle this case.
The draft RFC for improving the HTTP/1.1 standard, says that you don't
need to support chunked requests to be HTTP/1.1 compliant. Kevin
linked it earlier but here it is again:
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-22#section-3.3.3
The important text is
[[[
A server MAY reject a request that contains a message body but not a
Content-Length by responding with 411 (Length Required).
]]]
The chair of the working group posted this:
http://www.mnot.net/blog/2011/07/11/what_proxies_must_do
Obviously, it would be nice if everyone supported chunked requests.
But the reality is that hasn't fully happened. RFC 2616 left the door
open to HTTP/1.1 servers to not support chunked requests by providing
the 411 status response. Eventually the HTTPbis draft will become an
RFC and these ambiguities will be entirely removed. As such I assert
that the HTTP/1.1 standard does allow this behavior and that these
proxies are not broken, busted, non-compliant or whatever phrase you
want to use to that effect. (Yes I originally thought chunked was
required, but the above two links have convinced me otherwise).
It's hard to argue that we're following the HTTP/1.1 standard when our
behavior when proxied through a server that is exercising it's option
to not implement chunked requests is to fail. A fundamental feature
of HTTP is negotiation to prevent these sorts of failures so that
everyone can independently implement the standard.
> "Prefer" is not the same as "must" :-)
This recommendation is a suggestion to avoid the very problem we're
talking about here.
Specifically it says (again from the IETF link above):
[[[
Unless a transfer coding other than chunked has been applied, a
client that sends a request containing a message body SHOULD use a
valid Content-Length header field if the message body length is known
in advance, rather than the chunked transfer coding, since some
existing services respond to chunked with a 411 (Length Required)
status code even though they understand the chunked transfer coding.
This is typically because such services are implemented via a gateway
that requires a content-length in advance of being called and the
server is unable or unwilling to buffer the entire request before
processing.
]]]
This is flat out the "be liberal with what you accept and conservative
with what you send policy". We've now been bitten by the very issue
that this paragraph exists to warn us of. Are we really so arrogant
as to suggest that we should ignore this advice?
> In our current model, we prefer chunked. But there is a pretty
> straightforward extension to serf's bucket model that should allow us to use
> C-L in many situations. We *might* be able to do that in a serf 1.x, but I'm
> not sure. Worst case, we'll add the feature in serf 2.x.
Ivan has managed to implement this on Serf trunk. It seems that the
plan is now to include it in Serf 1.4.x which Ivan says should be out
in a month (not holding my breath but still relatively soon).
Given this development I'm wondering if the option makes sense at all.
Shouldn't we just eat the extra round trip to detect this issue and
then when we build against Serf 1.4.x simply stop issuing the extra
request to detect it?
Which brings us to Ivan's argument, which is that the cost is too
expensive to just detect this always.
On Tue, Jun 25, 2013 at 3:21 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> Please note that this extra request is per session and currently we
> create many sessions even during one operation. And I'm also not happy
> to make performance worse for users who doesn't use reverse proxies
> and etc.
First and foremost I'd argue that this is not backed by any
performance measurements. We have the feature implemented. How bad
does it really impact performance.
We limit RA serf to a max of 8 connections, but with things like
svn:externals we open new connections and then close them, so 8 may
not be the absolute worst case scenario.
[[[
#define SVN_RA_SERF__MAX_CONNECTIONS_LIMIT 8
]]]
We also require that we can make at least 2 connections to the server.
So it's hard to say what the typical behavior would be across so many
different configurations. But worst case scenario is always going to
be number_connections_opened * round_trip_time (RTT).
The corporate users that Ivan is worried about that are likely to have
no issues here because they have no such proxy in their way are the
ones likely to have the lowest RTT.
So let's say you have a RTT of 500ms, that means your command is going
to be delayed by 4 seconds assuming 8 connections, it also means that
you're going to have a pretty bad time using Subversion anyway. Yes
that sucks, but I doubt very many of our commands actually make use of
all 8 of those connections, remember we only actually require 2.
Right now from the US I have a RTT to svn.eu.apache.org of about 180ms
and to svn.us.apache.org of about 20ms, meaning worst case scenario
for those is a delay of 1.4 seconds and 160ms.
I have a very hard time believing that the above is worth the concern.
We should implement this detection behavior by default.
Given Ivan's work on Serf to prefer sending Content-Length, we can
disable it if we're built against a Serf that behaves this way.
I think that we should defer from adding an option to manipulate this
behavior until such a time that we find significant performance
issues. Especially if down the line we're not going to need it
because we can prefer CL.
If we feel that we MUST have the option then I think a different name
should be selected. I realize that Greg pushed for this name in order
to encourage proxy admins to fix their proxies to support chunked
requests. But that name seems like a huge mistake in light of the
trend towards HTTP/1.1 explicitly allowing servers to behave this way.
Ignoring any issues with the busted part of "busted-proxy" it also
doesn't follow our naming conventions. We have several http-proxy-*
options. I realize these are for client forward requests, but if
we're going to add an option we should follow our naming conventions,
meaning at a minimum http- should be prefixed. If we must stay with
this "busted-proxy" name then "http-proxy-busted" is a better match to
what we have. However, i think that risks confusing users who think
it only applies when they have a forward proxy configured.
I'd suggest http-detect-chunking with a description of "Detect if the
server supports chunked requests. This may be necessary for use when
a proxy does not support chunked requests. By default this is off
because mod_dav_svn always supports chunked requests and detection of
this hurts performance."
I'll look some more tomorrow into other options here with respect to a
better way to detect this but I'm having a hard time believe are
options are anything other than 1) extra request, 2) don't use chunked
requests, 3) punt on supporting these proxies.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jun 25, 2013 at 3:11 PM, <km...@rockwellcollins.com> wrote:
> > > I agree that force-http10 is not good name and semantic. Actually
> > > these proxies is not busted: it's allowed to HTTP/1.1 proxies to
> > > require content-length if they want. And strictly speaking proxies may
> > > have different behavior for different requests.
> >
> > From *our* standpoint, they are busted. Subversion wants to use
> > chunked requests. If they don't support it, then they are busted.
> > Simple as that.
> >
> > And we want to use a provocative name so that people understand
> > something needs to be *fixed*. Fixed for us because we view them as
> > *busted*.
>
> From the *users* standpoint subversion is busted. Something that
>
I'm not seeing the point. Subversion will (now) work, but we still view the
proxy as busted. It doesn't provide the performance characteristics that
Subversion wants and expects. Note that Subversion is built to work against
mod_dav_svn which is HTTP/1.1 with chunked requests. The interposition of a
proxy that disables chunked requests... busted.
> worked fine in 1.7 does not work in 1.8 without modifying potentially
> unrelated things that neither the server admin or the client
> user may have control over. (Think proxy at a hotel, etc.)
>
Of course. But we can fix this. (and I believe, have fixed it)
> The spec states that 411 is an allowed response and is it also states
> the client should prefer to not use chunked requests if possible.
> Serf is being overly optimistic here.
>
"Prefer" is not the same as "must" :-)
In our current model, we prefer chunked. But there is a pretty
straightforward extension to serf's bucket model that should allow us to
use C-L in many situations. We *might* be able to do that in a serf 1.x,
but I'm not sure. Worst case, we'll add the feature in serf 2.x.
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c
ra_serf.h serf.c util.c
Posted by km...@rockwellcollins.com.
> > I agree that force-http10 is not good name and semantic. Actually
> > these proxies is not busted: it's allowed to HTTP/1.1 proxies to
> > require content-length if they want. And strictly speaking proxies may
> > have different behavior for different requests.
>
> From *our* standpoint, they are busted. Subversion wants to use
> chunked requests. If they don't support it, then they are busted.
> Simple as that.
>
> And we want to use a provocative name so that people understand
> something needs to be *fixed*. Fixed for us because we view them as
> *busted*.
>From the *users* standpoint subversion is busted. Something that
worked fine in 1.7 does not work in 1.8 without modifying potentially
unrelated things that neither the server admin or the client
user may have control over. (Think proxy at a hotel, etc.)
The spec states that 411 is an allowed response and is it also states
the client should prefer to not use chunked requests if possible.
Serf is being overly optimistic here.
If this were a 2.x change it wouldn't be so concerning because there
isn't the backwards compatibility guarantee...
Kevin R.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jun 25, 2013 at 8:15 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Sun, Jun 23, 2013 at 3:43 AM, Greg Stein <gs...@gmail.com> wrote:
>> On Fri, Jun 21, 2013 at 11:36 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>...
>>> The fix seems to be pretty simple and could be safely backported (see
>>> patch). The only problem that probably we cannot backport addition of
>>> new configuration option due our backward compatibility policy. But
>>> may be I'm wrong.
>>
>> The SVN_CONFIG_OPTION_HTTP_FORCE_HTTP10 symbol cannot be added.
>> Anything using and compiled against that could not be compiled against
>> 1.8.0.
>>
>> But: we don't have to add the public symbol. We can certainly read/use
>> the config option in a 1.8.1 release.
>>
>> But as I said else-thread, I think we should only disable chunked
>> requests. Not force HTTP/1.0 completely. And call it busted-proxy :-)
>>
> I agree that force-http10 is not good name and semantic. Actually
> these proxies is not busted: it's allowed to HTTP/1.1 proxies to
> require content-length if they want. And strictly speaking proxies may
> have different behavior for different requests.
>From *our* standpoint, they are busted. Subversion wants to use
chunked requests. If they don't support it, then they are busted.
Simple as that.
And we want to use a provocative name so that people understand
something needs to be *fixed*. Fixed for us because we view them as
*busted*.
> So I'm going to commit my patch with 'http-disable-chunked-requests'
That name does not signify that a problem exists which needs to be fixed.
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sun, Jun 23, 2013 at 3:43 AM, Greg Stein <gs...@gmail.com> wrote:
> On Fri, Jun 21, 2013 at 11:36 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>...
>> The fix seems to be pretty simple and could be safely backported (see
>> patch). The only problem that probably we cannot backport addition of
>> new configuration option due our backward compatibility policy. But
>> may be I'm wrong.
>
> The SVN_CONFIG_OPTION_HTTP_FORCE_HTTP10 symbol cannot be added.
> Anything using and compiled against that could not be compiled against
> 1.8.0.
>
> But: we don't have to add the public symbol. We can certainly read/use
> the config option in a 1.8.1 release.
>
> But as I said else-thread, I think we should only disable chunked
> requests. Not force HTTP/1.0 completely. And call it busted-proxy :-)
>
I agree that force-http10 is not good name and semantic. Actually
these proxies is not busted: it's allowed to HTTP/1.1 proxies to
require content-length if they want. And strictly speaking proxies may
have different behavior for different requests.
So I'm going to commit my patch with 'http-disable-chunked-requests'
and nominate for backport. This should provide workaround for current
users. And then continue discussion about full/automatic solution.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Jun 21, 2013 at 11:36 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>...
> The fix seems to be pretty simple and could be safely backported (see
> patch). The only problem that probably we cannot backport addition of
> new configuration option due our backward compatibility policy. But
> may be I'm wrong.
The SVN_CONFIG_OPTION_HTTP_FORCE_HTTP10 symbol cannot be added.
Anything using and compiled against that could not be compiled against
1.8.0.
But: we don't have to add the public symbol. We can certainly read/use
the config option in a 1.8.1 release.
But as I said else-thread, I think we should only disable chunked
requests. Not force HTTP/1.0 completely. And call it busted-proxy :-)
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Fri, Jun 21, 2013 at 7:14 PM, Greg Stein <gs...@gmail.com> wrote:
>
> On Jun 21, 2013 10:36 AM, "Ivan Zhakov" <iv...@visualsvn.com> wrote:
>>....
>> >> Problem with this approach that some servers may support HTTP/1.1
>> >> partially. I.e. declare them as HTTP/1.1 but do not support chunked
>> >> Transfer-Encoding.
>> >
>> > Then fix that problem. Add a flag saying "busted_http11" and make it
>> > send requests with Content-Length after that.
>> >
>> Do you mean run-time configuration option like "http-force-http10 = yes"?
>
> Oh. Hadn't thought about that. I was thinking dynamic detection.
>
> I would favor dynamic over Yet Another Config Option, but you're the one
> writing this code right now :-)
>
I'm also favor dynamic detection over config option. But I don't see
good way to detect such busted proxies without introducing another
request or risk breaking something when request completed in different
order.
I suggest add config option as immediate fix, because problem reported
at least five times.
The fix seems to be pretty simple and could be safely backported (see
patch). The only problem that probably we cannot backport addition of
new configuration option due our backward compatibility policy. But
may be I'm wrong.
[[[[
Add new 'http-force-http10' configuration option to force using HTTP/1.0
even server declares HTTP/1.1 support.
* subversion/include/svn_config.h
(SVN_CONFIG_OPTION_HTTP_FORCE_HTTP10): New.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__session_t): Add FORCE_HTTP10 flag.
* subversion/libsvn_ra_serf/serf.c
(load_config): Parse 'http-force-http10' configuration option.
* subversion/libsvn_ra_serf/util.c
(handle_response): Do not upgrade to HTTP/1.1 if force_http10 is true.
* subversion/libsvn_subr/config_file.c
(svn_config_ensure): Mention http-force-http10 in 'servers' file template.
]]]]
(I'm not sure about option name)
PS: I'm going to country until Tuesday, so feel free to commit and
backport this patch.
--
Ivan Zhakov
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Jun 21, 2013 10:36 AM, "Ivan Zhakov" <iv...@visualsvn.com> wrote:
>....
> >> Problem with this approach that some servers may support HTTP/1.1
> >> partially. I.e. declare them as HTTP/1.1 but do not support chunked
> >> Transfer-Encoding.
> >
> > Then fix that problem. Add a flag saying "busted_http11" and make it
> > send requests with Content-Length after that.
> >
> Do you mean run-time configuration option like "http-force-http10 = yes"?
Oh. Hadn't thought about that. I was thinking dynamic detection.
I would favor dynamic over Yet Another Config Option, but you're the one
writing this code right now :-)
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Fri, Jun 21, 2013 at 5:51 PM, Greg Stein <gs...@gmail.com> wrote:
> On Fri, Jun 21, 2013 at 9:45 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>> On Fri, Jun 21, 2013 at 9:19 AM, Greg Stein <gs...@gmail.com> wrote:
>>>> Please revert this. You *cannot* rely on a 411 response.
>>>>
>>>> You are also not allowed to send a 1.1 request to a server for which
>>>> you don't know if they support 1.0 vs 1.1.
>>
>> On Fri, Jun 21, 2013 at 5:28 PM, Greg Stein <gs...@gmail.com> wrote:
>>> This has been quoted before, but I'll repeat it again. RFC 2145, Section 2:
>>>
>>> "One consequence of these rules is that an HTTP/1.1 message sent to an
>>> HTTP/1.0 recipient (or a recipient whose version is unknown) MUST be
>>> constructed so that it remains a valid HTTP/1.0 message when all
>>> headers not defined in the HTTP/1.0 specification [1] are removed."
>>>
>>> Removing the Transfer-Coding: chunked header completely alters the
>>> message. Thus, it cannot be sent to a 1.0 server. This is why you
>>> cannot rely on any particular behavior or response status.
>>
>> Hi Greg! Thanks for review.
>>
>> My patch doesn't rely on 411 response only: ra_serf will downgrade to
>> HTTP/1.0 if *any* HTTP/1.0 response received *or* HTTP/1.1 411 status
>> code.
>
> We've seen 501 responses. We've seen other weird responses in the
> past. I think we even crashed some servers, iirc.
>
> But it just doesn't matter. You're not allowed to send a request like
> that. Period.
>
>> I've tested several browsers and they use HTTP/1.1 as first request.
>
> Doesn't matter what they do. Maybe they already know the target
> supports HTTP/1.1. Who knows? Who cares?
>
> RFC 2145 is very clear here. We've seen problems when we did not obey
> the rules. I spent a good bit of time fixing our code to obey them, in
> order to get this stuff working.
>
Reverted in r1495455.
>>...
>> Problem with this approach that some servers may support HTTP/1.1
>> partially. I.e. declare them as HTTP/1.1 but do not support chunked
>> Transfer-Encoding.
>
> Then fix that problem. Add a flag saying "busted_http11" and make it
> send requests with Content-Length after that.
>
Do you mean run-time configuration option like "http-force-http10 = yes"?
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Jun 21, 2013 at 9:45 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On Fri, Jun 21, 2013 at 9:19 AM, Greg Stein <gs...@gmail.com> wrote:
>>> Please revert this. You *cannot* rely on a 411 response.
>>>
>>> You are also not allowed to send a 1.1 request to a server for which
>>> you don't know if they support 1.0 vs 1.1.
>
> On Fri, Jun 21, 2013 at 5:28 PM, Greg Stein <gs...@gmail.com> wrote:
>> This has been quoted before, but I'll repeat it again. RFC 2145, Section 2:
>>
>> "One consequence of these rules is that an HTTP/1.1 message sent to an
>> HTTP/1.0 recipient (or a recipient whose version is unknown) MUST be
>> constructed so that it remains a valid HTTP/1.0 message when all
>> headers not defined in the HTTP/1.0 specification [1] are removed."
>>
>> Removing the Transfer-Coding: chunked header completely alters the
>> message. Thus, it cannot be sent to a 1.0 server. This is why you
>> cannot rely on any particular behavior or response status.
>
> Hi Greg! Thanks for review.
>
> My patch doesn't rely on 411 response only: ra_serf will downgrade to
> HTTP/1.0 if *any* HTTP/1.0 response received *or* HTTP/1.1 411 status
> code.
We've seen 501 responses. We've seen other weird responses in the
past. I think we even crashed some servers, iirc.
But it just doesn't matter. You're not allowed to send a request like
that. Period.
> I've tested several browsers and they use HTTP/1.1 as first request.
Doesn't matter what they do. Maybe they already know the target
supports HTTP/1.1. Who knows? Who cares?
RFC 2145 is very clear here. We've seen problems when we did not obey
the rules. I spent a good bit of time fixing our code to obey them, in
order to get this stuff working.
>...
> Problem with this approach that some servers may support HTTP/1.1
> partially. I.e. declare them as HTTP/1.1 but do not support chunked
> Transfer-Encoding.
Then fix that problem. Add a flag saying "busted_http11" and make it
send requests with Content-Length after that.
But the answer is *not* to start a connection using 1.0.
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Sun, Jun 23, 2013 at 7:35 PM, Peter Samuelson <pe...@p12n.org> wrote:
>
>> If you really want to push, then "proxy-hates-chunks" could work well.
>
> Oh man. Not "proxy-blows-chunks"? (SCNR.)
LOL!
Actually, after an offlist email with Daniel, I would like to "insist"
on just calling it "busted-proxy". We don't need to be fine-grained on
a config option that is only used for edge/broken cases.
Consider the counter-example, where we have three proxy-related
problems. We create *three* configuration options, document all three,
and implement all three across ra_serf. And they are all edge cases.
That is a lot of documentation and work for edge cases. And why do we
need to solve proxy problem #2, and *avoid* hauling in solutions for
#1 and #3? To optimize the user experience in a non-ideal situation?
[by optimizing use of the (busted) proxy]
I believe that the right answer is that we would take all three proxy
issues and lump them all under "busted-proxy". Even if the target
proxy doesn't suffer issue #3, it doesn't really hurt to throw in a
solution for that, even though the user is only experiencing issue #1.
And shoot... let's say that we *do* find a serious issue with a proxy,
which we don't want to haul in when a user is only experiencing
lack-of-chunked-requests. Then we just change busted-proxy from on/off
to on/off/horribly-wrong. Or maybe *that* proxy problem gets its own
config option. Unless/until, then I really think a single catch-all is
the best approach.
To be concrete, we've suggested sending a second OPTIONS request to
detect lack of support for chunked requests. Two years from now, we
discover another proxy problem and instruct the user to turn on
busted-proxy. Is it really a hardship for *that* user to have a second
OPTIONS in their startup logic? Nah.
The user will work to fix the proxy issue, or lobby for its solution.
While that happens, they will suffer a bit of degradation under the
overall "busted-proxy" flag. This isn't an area that we need to
partition and discretely solve. Especially since it "shouldn't" exist
in the first place.
Cheers,
-g
Re: svn commit: r1495419 - in
/subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c
util.c
Posted by Peter Samuelson <pe...@p12n.org>.
> If you really want to push, then "proxy-hates-chunks" could work well.
Oh man. Not "proxy-blows-chunks"? (SCNR.)
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Jun 23, 2013 4:54 AM, "Daniel Shahaf" <da...@apache.org> wrote:
>
> On Sat, Jun 22, 2013 at 07:39:23PM -0400, Greg Stein wrote:
> > The flag could be something like "busted-proxy = on". When you start
> > seeing blog posts like "In order to checkout from EXAMPLE.COM, you
> > must set busted-proxy=on in your .subversion configuration", and that
> > gets back to EXAMPLE.COM ... you can bet they'll dig into the problem
> > and fix it :-)
>
> We should use a more specific name, for the same reason that we don't add
new
> meanings to --force.
I think I disagree, as this is pretty specific already. "force" has zero
semantic meaning/association. But in this case: yes, the proxy is busted
from our view. We have to degrade to work with it.
If you really want to push, then "proxy-hates-chunks" could work well.
Our goal is to get those proxies to handle chunked requests. Politically
correct terminology is not very goal-oriented. Throw in some bitchiness, I
say.
Cheers,
-g
Re: svn commit: r1495419 - in
/subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c
util.c
Posted by Daniel Shahaf <da...@apache.org>.
On Sat, Jun 22, 2013 at 07:39:23PM -0400, Greg Stein wrote:
> The flag could be something like "busted-proxy = on". When you start
> seeing blog posts like "In order to checkout from EXAMPLE.COM, you
> must set busted-proxy=on in your .subversion configuration", and that
> gets back to EXAMPLE.COM ... you can bet they'll dig into the problem
> and fix it :-)
We should use a more specific name, for the same reason that we don't add new
meanings to --force.
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Sat, Jun 22, 2013 at 5:17 PM, Johan Corveleyn <jc...@gmail.com> wrote:
> On Sat, Jun 22, 2013 at 11:12 AM, Lieven Govaerts <sv...@mobsol.be> wrote:
>> On Sat, Jun 22, 2013 at 10:24 AM, Johan Corveleyn <jc...@gmail.com> wrote:
> ...
>>> Maybe I'm missing something, but why would you downgrade to HTTP/1.0
>>> only because of the 411 (Content Length Required)? Can't you just
>>> continue using HTTP/1.1, but use CL instead of chunked requests? Or is
>>> falling back to HTTP/1.0 just the quickest solution within the current
>>> implementation?
>>
>> There are only 2 differences between 'http 1.0 mode' and 'http 1.1
>> mode' in ra_serf. First is using Content-Length instead of chunked
>> encoding. The second is that the Connection:keep-alive header is set
>> on all responses.
Hmm. We should probably stick with the Keep-Alive even on 1.0
requests. Some proxies understand that.
>> I actually expected a third difference, pipelining should be disabled
>> in http 1.0 mode via set_max_outstanding_requests call, but I don't
>> see that in the code. gstein?
HTTP/1.0 can do pipelining, so I see no reason to disable that.
>>> IIUC HTTP/1.1 has other advantages besides chunked encoding, and can
>>> be used perfectly well without it. So it seems you can fall back to
>>> HTTP/1.1 + ContentLength here, no?
Right. We could use compression, for example. (iirc, 1.0 doesn't have that)
>> Yes, that's indeed possible. It does add yet another variant of
>> communication though. IMO we already have more options than we can
>> support, we should cut down in the multitude of ways we support legacy
>> systems.
I'm thinking we just have two flags: http10 (aka "don't know whether
the server is 1.0 or 1.1") and "disable_chunked_requests" (or some
shorter name).
>...
>>>> This will add one round-trip of overhead for every serf session, and
>>>> it impacts all users, even those that are not behind such (outdated)
>>>> proxies.
If you send a second request, then I'd suggest just repeating the
OPTIONS rather than a PROPFIND. It will be much easier on the server.
>>>> I'd support making this dependent on a 'http1.0_server" option in the
>>>> servers runtime configuration. With this option it only impacts those
>>>> users that are working behind outdated proxies. We can include the
>>>> necessary instructions for users in the error description of the
>>>> by-default-unexpected 411 response.
>>>>
>>>> This should also be a clear trigger for hosting companies to upgrade
>>>> their proxy infra.
The flag could be something like "busted-proxy = on". When you start
seeing blog posts like "In order to checkout from EXAMPLE.COM, you
must set busted-proxy=on in your .subversion configuration", and that
gets back to EXAMPLE.COM ... you can bet they'll dig into the problem
and fix it :-)
[ as an aside, I complained to an svn hosting company that their proxy
config broke serf; they were in transition and accidentally busted it;
they had it fixed a few days later ]
I would be in favor of instructions on how to fix these 411 errors,
then have the user set the flag and try again.
Now: the failure mode when the server *does* fix the flag, is a second
OPTIONS is sent when it doesn't need to be. Not a horrible problem.
>>> I'd favor always auto-detecting, even at the cost of the extra
>>> round-trip. Or perhaps make the default auto-detection, and have a
>>> flag to shortcut the auto-detection if you know what you're doing.
>>
>> That'd be the 'http1.0_server' flag, or the cached flag I've mentioned
>> in my other mail.
>
> I'm not really fond of a runtime config flag for this, but then again
> I'm also not fond of forcing the extra roundtrip for all users on the
> account of these wonky proxies (which I suppose are really a small
> minority).
We should *not* send an extra request, unless the user specifically
instructs us to. In a properly-working environment, it would not be
needed. That is our typical and ideal situation. Users/admins will
need to compensate for their broken setup. It isn't really our
problem, but we'll help a little (a flag) for them to work through it.
> The cached flag sounds to me like a risky solution (in the sense that
> it may become a source of bugs and strange behavior). What if a proxy
> gets upgraded (or configuration changed, for better (enabling chunked
> encoding) or for worse (disabling it))? How will the cache be
> invalidated? If it's not invalidated in some automatic way, a client
> that has "use 1.0" cached for a particular server will continue using
> HTTP/1.0 forever, even if the server / proxy gets upgraded, ... If
> it's the other way around, the user will suddenly start getting errors
> (so the error message still better point out the "manual override" or
> "clear cache" switch).
I'd suggest runtime checking (a second OPTIONS) based on a user
setting, or a cached flag. If we find that the host suddenly starts
supporting chunked requests, then we can clear the cache. We could
even warn the user "you set busted-proxy, but it appears to be working
now; you may want to clear that flag".
> At the moment I don't have any better ideas though. Perhaps we / you
> should wait a little more for other people to give their opinion or
> suggestions about this?
>
> How about Kevin's suggestion [1], that serf should ideally be using CL
> instead of chunked-encoding for the requests?
As a default? Absolutely not.
In a future serf where we change the bucket model to include a
"getsize" method, then we could use C-L when the bucket reports a
known/fixed size. Some buckets may not be able to do that, of course,
and we defer to chunked requests.
Ideally, ra_serf should be implementing some custom buckets,
corresponding to different types of requests it makes. We could
dynamically build the request, which would (typically) be much more
efficient (on-demand/lazy reading and lower memory overhead).
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Johan Corveleyn <jc...@gmail.com>.
On Sat, Jun 22, 2013 at 11:12 AM, Lieven Govaerts <sv...@mobsol.be> wrote:
> On Sat, Jun 22, 2013 at 10:24 AM, Johan Corveleyn <jc...@gmail.com> wrote:
...
>> Maybe I'm missing something, but why would you downgrade to HTTP/1.0
>> only because of the 411 (Content Length Required)? Can't you just
>> continue using HTTP/1.1, but use CL instead of chunked requests? Or is
>> falling back to HTTP/1.0 just the quickest solution within the current
>> implementation?
>
> There are only 2 differences between 'http 1.0 mode' and 'http 1.1
> mode' in ra_serf. First is using Content-Length instead of chunked
> encoding. The second is that the Connection:keep-alive header is set
> on all responses.
>
> I actually expected a third difference, pipelining should be disabled
> in http 1.0 mode via set_max_outstanding_requests call, but I don't
> see that in the code. gstein?
>
>> IIUC HTTP/1.1 has other advantages besides chunked encoding, and can
>> be used perfectly well without it. So it seems you can fall back to
>> HTTP/1.1 + ContentLength here, no?
>
> Yes, that's indeed possible. It does add yet another variant of
> communication though. IMO we already have more options than we can
> support, we should cut down in the multitude of ways we support legacy
> systems.
Okay, fair enough. You're the one doing the work :-), so I have little
argument that you should make it more complex than necessary. The most
important thing is that we can handle these weird acting servers/proxies at all.
>>> This will add one round-trip of overhead for every serf session, and
>>> it impacts all users, even those that are not behind such (outdated)
>>> proxies.
>>>
>>> I'd support making this dependent on a 'http1.0_server" option in the
>>> servers runtime configuration. With this option it only impacts those
>>> users that are working behind outdated proxies. We can include the
>>> necessary instructions for users in the error description of the
>>> by-default-unexpected 411 response.
>>>
>>> This should also be a clear trigger for hosting companies to upgrade
>>> their proxy infra.
>>
>> I'd favor always auto-detecting, even at the cost of the extra
>> round-trip. Or perhaps make the default auto-detection, and have a
>> flag to shortcut the auto-detection if you know what you're doing.
>
> That'd be the 'http1.0_server' flag, or the cached flag I've mentioned
> in my other mail.
I'm not really fond of a runtime config flag for this, but then again
I'm also not fond of forcing the extra roundtrip for all users on the
account of these wonky proxies (which I suppose are really a small
minority).
The cached flag sounds to me like a risky solution (in the sense that
it may become a source of bugs and strange behavior). What if a proxy
gets upgraded (or configuration changed, for better (enabling chunked
encoding) or for worse (disabling it))? How will the cache be
invalidated? If it's not invalidated in some automatic way, a client
that has "use 1.0" cached for a particular server will continue using
HTTP/1.0 forever, even if the server / proxy gets upgraded, ... If
it's the other way around, the user will suddenly start getting errors
(so the error message still better point out the "manual override" or
"clear cache" switch).
At the moment I don't have any better ideas though. Perhaps we / you
should wait a little more for other people to give their opinion or
suggestions about this?
How about Kevin's suggestion [1], that serf should ideally be using CL
instead of chunked-encoding for the requests?
[1] http://svn.haxx.se/dev/archive-2013-06/0518.shtml
--
Johan
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Lieven Govaerts <sv...@mobsol.be>.
On Sat, Jun 22, 2013 at 10:24 AM, Johan Corveleyn <jc...@gmail.com> wrote:
> On Sat, Jun 22, 2013 at 10:07 AM, Lieven Govaerts <sv...@mobsol.be> wrote:
>> On Sat, Jun 22, 2013 at 9:08 AM, Philip Martin
>> <ph...@wandisco.com> wrote:
>>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>>
>>>> Problem with this approach that some servers may support HTTP/1.1
>>>> partially. I.e. declare them as HTTP/1.1 but do not support chunked
>>>> Transfer-Encoding.
>>>>
>>>> I wanted to avoid downgrade to HTTP/1.0 on later requests because it
>>>> could introduce requests ordering issue. For example:
>>>> C: HTTP/1.0 OPTIONS /
>>>> S: HTTP/1.1 Response
>>>>
>>>> C: HTTP/1.1 PROPFIND
>>>> C: HTTP/1.1 GET
>>>> S: HTTP/1.1 411 Length required
>>>> S: HTTP/1.0 200 OK
>>>>
>>>> Then client send PROPFIND request again:
>>>> C: HTTP/1.0 PROPFIND
>>>> S: HTTP/1.1 200 OK
>>>>
>>>> But requests was completed and different order than our update editor
>>>> expected and most likely crash :(
>>>
>>> Suppose serf were to keep track of the number of outstanding requests
>>> (it may already do that I haven't checked). Then if the number of
>>> outstanding requests is zero when the 411 is received the downgrade to
>>> HTTP/1.0 will be OK. Lots of client operations start with multiple
>>> requests in serial before switching to pipelined requests, so in all
>>> those cases the downgrade will work.
>>
>> We need a solution for all cases, most is not enough.
>>
>> Given the requirement of the first request's mandatory http/1.0 and
>> strict ordering of req/resp in pipeline mode, I don't see how we can
>> identify both http/1.1 compliance and chunked encoding support with
>> only one non-pipelined request.
>>
>> Why not play it safe then and send one extra request on the first
>> connection in the context?
>>
>> The first request is a HTTP/1.0 request to detect if the connection
>> can be upgraded to HTTP/1.1.
>> The second request (the PROPFIND) is sent using chunked encoding to
>> detect if that's supported.
>> If 411 is received fallback to HTTP/1.0 mode, otherwise continue with
>> HTTP/1.1 + chunked encoding.
>
> Maybe I'm missing something, but why would you downgrade to HTTP/1.0
> only because of the 411 (Content Length Required)? Can't you just
> continue using HTTP/1.1, but use CL instead of chunked requests? Or is
> falling back to HTTP/1.0 just the quickest solution within the current
> implementation?
There are only 2 differences between 'http 1.0 mode' and 'http 1.1
mode' in ra_serf. First is using Content-Length instead of chunked
encoding. The second is that the Connection:keep-alive header is set
on all responses.
I actually expected a third difference, pipelining should be disabled
in http 1.0 mode via set_max_outstanding_requests call, but I don't
see that in the code. gstein?
> IIUC HTTP/1.1 has other advantages besides chunked encoding, and can
> be used perfectly well without it. So it seems you can fall back to
> HTTP/1.1 + ContentLength here, no?
Yes, that's indeed possible. It does add yet another variant of
communication though. IMO we already have more options than we can
support, we should cut down in the multitude of ways we support legacy
systems.
>> This will add one round-trip of overhead for every serf session, and
>> it impacts all users, even those that are not behind such (outdated)
>> proxies.
>>
>> I'd support making this dependent on a 'http1.0_server" option in the
>> servers runtime configuration. With this option it only impacts those
>> users that are working behind outdated proxies. We can include the
>> necessary instructions for users in the error description of the
>> by-default-unexpected 411 response.
>>
>> This should also be a clear trigger for hosting companies to upgrade
>> their proxy infra.
>
> I'd favor always auto-detecting, even at the cost of the extra
> round-trip. Or perhaps make the default auto-detection, and have a
> flag to shortcut the auto-detection if you know what you're doing.
That'd be the 'http1.0_server' flag, or the cached flag I've mentioned
in my other mail.
> --
> Johan
Lieven
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Johan Corveleyn <jc...@gmail.com>.
On Sat, Jun 22, 2013 at 10:07 AM, Lieven Govaerts <sv...@mobsol.be> wrote:
> On Sat, Jun 22, 2013 at 9:08 AM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>
>>> Problem with this approach that some servers may support HTTP/1.1
>>> partially. I.e. declare them as HTTP/1.1 but do not support chunked
>>> Transfer-Encoding.
>>>
>>> I wanted to avoid downgrade to HTTP/1.0 on later requests because it
>>> could introduce requests ordering issue. For example:
>>> C: HTTP/1.0 OPTIONS /
>>> S: HTTP/1.1 Response
>>>
>>> C: HTTP/1.1 PROPFIND
>>> C: HTTP/1.1 GET
>>> S: HTTP/1.1 411 Length required
>>> S: HTTP/1.0 200 OK
>>>
>>> Then client send PROPFIND request again:
>>> C: HTTP/1.0 PROPFIND
>>> S: HTTP/1.1 200 OK
>>>
>>> But requests was completed and different order than our update editor
>>> expected and most likely crash :(
>>
>> Suppose serf were to keep track of the number of outstanding requests
>> (it may already do that I haven't checked). Then if the number of
>> outstanding requests is zero when the 411 is received the downgrade to
>> HTTP/1.0 will be OK. Lots of client operations start with multiple
>> requests in serial before switching to pipelined requests, so in all
>> those cases the downgrade will work.
>
> We need a solution for all cases, most is not enough.
>
> Given the requirement of the first request's mandatory http/1.0 and
> strict ordering of req/resp in pipeline mode, I don't see how we can
> identify both http/1.1 compliance and chunked encoding support with
> only one non-pipelined request.
>
> Why not play it safe then and send one extra request on the first
> connection in the context?
>
> The first request is a HTTP/1.0 request to detect if the connection
> can be upgraded to HTTP/1.1.
> The second request (the PROPFIND) is sent using chunked encoding to
> detect if that's supported.
> If 411 is received fallback to HTTP/1.0 mode, otherwise continue with
> HTTP/1.1 + chunked encoding.
Maybe I'm missing something, but why would you downgrade to HTTP/1.0
only because of the 411 (Content Length Required)? Can't you just
continue using HTTP/1.1, but use CL instead of chunked requests? Or is
falling back to HTTP/1.0 just the quickest solution within the current
implementation?
IIUC HTTP/1.1 has other advantages besides chunked encoding, and can
be used perfectly well without it. So it seems you can fall back to
HTTP/1.1 + ContentLength here, no?
> This will add one round-trip of overhead for every serf session, and
> it impacts all users, even those that are not behind such (outdated)
> proxies.
>
> I'd support making this dependent on a 'http1.0_server" option in the
> servers runtime configuration. With this option it only impacts those
> users that are working behind outdated proxies. We can include the
> necessary instructions for users in the error description of the
> by-default-unexpected 411 response.
>
> This should also be a clear trigger for hosting companies to upgrade
> their proxy infra.
I'd favor always auto-detecting, even at the cost of the extra
round-trip. Or perhaps make the default auto-detection, and have a
flag to shortcut the auto-detection if you know what you're doing.
--
Johan
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 06/23/2013 07:47 PM, Greg Stein wrote:
> On Sun, Jun 23, 2013 at 3:56 PM, Blair Zajac <bl...@orcaware.com> wrote:
>> On 6/22/13 1:22 AM, Lieven Govaerts wrote:
>>> Even better if we could cache this info somewhere automatically. So
>>> that the first time svn connects to a server ever it will use this
>>> 'slow' procedure, and then persists the results http/1.0 vs http/1.1
>>> and chunked-encoding supported somewhere. Like we already do for
>>> passwords and ssl certificate validation.
>>
>> Will this work if you change where you are connecting from, e.g. at home one
>> day and then from a Starbucks or maybe your work where you have different
>> proxies in the path?
>
> Well... I don't think that we're talking (yet) about a cache. Just a
> config and an extra detection step. In your scenario, you'll suffer a
> slightly slower start cost (the extra OPTIONS), and in the other, a
> somewhat degraded experience (switching to C-L for all requests).
>
> But it should auto-switch as you move about.
>
> (assuming we don't try to cache, at this point)
Sounds good. Sounded like there was an idea of caching, which seemed
like it could be problematic.
Regards,
Blair
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Sun, Jun 23, 2013 at 3:56 PM, Blair Zajac <bl...@orcaware.com> wrote:
> On 6/22/13 1:22 AM, Lieven Govaerts wrote:
>> Even better if we could cache this info somewhere automatically. So
>> that the first time svn connects to a server ever it will use this
>> 'slow' procedure, and then persists the results http/1.0 vs http/1.1
>> and chunked-encoding supported somewhere. Like we already do for
>> passwords and ssl certificate validation.
>
> Will this work if you change where you are connecting from, e.g. at home one
> day and then from a Starbucks or maybe your work where you have different
> proxies in the path?
Well... I don't think that we're talking (yet) about a cache. Just a
config and an extra detection step. In your scenario, you'll suffer a
slightly slower start cost (the extra OPTIONS), and in the other, a
somewhat degraded experience (switching to C-L for all requests).
But it should auto-switch as you move about.
(assuming we don't try to cache, at this point)
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 6/22/13 1:22 AM, Lieven Govaerts wrote:
> Even better if we could cache this info somewhere automatically. So
> that the first time svn connects to a server ever it will use this
> 'slow' procedure, and then persists the results http/1.0 vs http/1.1
> and chunked-encoding supported somewhere. Like we already do for
> passwords and ssl certificate validation.
Will this work if you change where you are connecting from, e.g. at home one day
and then from a Starbucks or maybe your work where you have different proxies in
the path?
Blair
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Lieven Govaerts <sv...@mobsol.be>.
On Sat, Jun 22, 2013 at 10:07 AM, Lieven Govaerts <sv...@mobsol.be> wrote:
> On Sat, Jun 22, 2013 at 9:08 AM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>
>>> Problem with this approach that some servers may support HTTP/1.1
>>> partially. I.e. declare them as HTTP/1.1 but do not support chunked
>>> Transfer-Encoding.
>>>
>>> I wanted to avoid downgrade to HTTP/1.0 on later requests because it
>>> could introduce requests ordering issue. For example:
>>> C: HTTP/1.0 OPTIONS /
>>> S: HTTP/1.1 Response
>>>
>>> C: HTTP/1.1 PROPFIND
>>> C: HTTP/1.1 GET
>>> S: HTTP/1.1 411 Length required
>>> S: HTTP/1.0 200 OK
>>>
>>> Then client send PROPFIND request again:
>>> C: HTTP/1.0 PROPFIND
>>> S: HTTP/1.1 200 OK
>>>
>>> But requests was completed and different order than our update editor
>>> expected and most likely crash :(
>>
>> Suppose serf were to keep track of the number of outstanding requests
>> (it may already do that I haven't checked). Then if the number of
>> outstanding requests is zero when the 411 is received the downgrade to
>> HTTP/1.0 will be OK. Lots of client operations start with multiple
>> requests in serial before switching to pipelined requests, so in all
>> those cases the downgrade will work.
>
> We need a solution for all cases, most is not enough.
>
> Given the requirement of the first request's mandatory http/1.0 and
> strict ordering of req/resp in pipeline mode, I don't see how we can
> identify both http/1.1 compliance and chunked encoding support with
> only one non-pipelined request.
>
> Why not play it safe then and send one extra request on the first
> connection in the context?
>
> The first request is a HTTP/1.0 request to detect if the connection
> can be upgraded to HTTP/1.1.
> The second request (the PROPFIND) is sent using chunked encoding to
> detect if that's supported.
> If 411 is received fallback to HTTP/1.0 mode, otherwise continue with
> HTTP/1.1 + chunked encoding.
>
> This will add one round-trip of overhead for every serf session, and
> it impacts all users, even those that are not behind such (outdated)
> proxies.
>
> I'd support making this dependent on a 'http1.0_server" option in the
> servers runtime configuration. With this option it only impacts those
> users that are working behind outdated proxies. We can include the
> necessary instructions for users in the error description of the
> by-default-unexpected 411 response.
Even better if we could cache this info somewhere automatically. So
that the first time svn connects to a server ever it will use this
'slow' procedure, and then persists the results http/1.0 vs http/1.1
and chunked-encoding supported somewhere. Like we already do for
passwords and ssl certificate validation.
> This should also be a clear trigger for hosting companies to upgrade
> their proxy infra.
>
>> --
>> Philip Martin | Subversion Committer
>> WANdisco | Non-Stop Data
>> www.wandisco.com
>
> Lieven
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Lieven Govaerts <sv...@mobsol.be>.
On Sat, Jun 22, 2013 at 9:08 AM, Philip Martin
<ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> Problem with this approach that some servers may support HTTP/1.1
>> partially. I.e. declare them as HTTP/1.1 but do not support chunked
>> Transfer-Encoding.
>>
>> I wanted to avoid downgrade to HTTP/1.0 on later requests because it
>> could introduce requests ordering issue. For example:
>> C: HTTP/1.0 OPTIONS /
>> S: HTTP/1.1 Response
>>
>> C: HTTP/1.1 PROPFIND
>> C: HTTP/1.1 GET
>> S: HTTP/1.1 411 Length required
>> S: HTTP/1.0 200 OK
>>
>> Then client send PROPFIND request again:
>> C: HTTP/1.0 PROPFIND
>> S: HTTP/1.1 200 OK
>>
>> But requests was completed and different order than our update editor
>> expected and most likely crash :(
>
> Suppose serf were to keep track of the number of outstanding requests
> (it may already do that I haven't checked). Then if the number of
> outstanding requests is zero when the 411 is received the downgrade to
> HTTP/1.0 will be OK. Lots of client operations start with multiple
> requests in serial before switching to pipelined requests, so in all
> those cases the downgrade will work.
We need a solution for all cases, most is not enough.
Given the requirement of the first request's mandatory http/1.0 and
strict ordering of req/resp in pipeline mode, I don't see how we can
identify both http/1.1 compliance and chunked encoding support with
only one non-pipelined request.
Why not play it safe then and send one extra request on the first
connection in the context?
The first request is a HTTP/1.0 request to detect if the connection
can be upgraded to HTTP/1.1.
The second request (the PROPFIND) is sent using chunked encoding to
detect if that's supported.
If 411 is received fallback to HTTP/1.0 mode, otherwise continue with
HTTP/1.1 + chunked encoding.
This will add one round-trip of overhead for every serf session, and
it impacts all users, even those that are not behind such (outdated)
proxies.
I'd support making this dependent on a 'http1.0_server" option in the
servers runtime configuration. With this option it only impacts those
users that are working behind outdated proxies. We can include the
necessary instructions for users in the error description of the
by-default-unexpected 411 response.
This should also be a clear trigger for hosting companies to upgrade
their proxy infra.
> --
> Philip Martin | Subversion Committer
> WANdisco | Non-Stop Data
> www.wandisco.com
Lieven
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Lieven Govaerts <sv...@mobsol.be> writes:
>
>>> It seems to be connected to mod_deflate. My server was loading
>>> mod_deflate (but not setting up any filters) and the import works with
>>> the client setting
>>>
>>> http-compression = no
>>>
>>> but fails with
>>>
>>> http-compression = yes
>>>
>>> Still investigating.
>>
>> Don't you need r1497551?
>
> I have that. If I configure nginx with "gzip = no" then lots of the
> regression tests now pass when run through nginx with my patch.
Some of the remaining failures are caused when
svn_ra_serf__get_inherited_props sends a REPORT request and receives:
HTTP/1.1 411 Length Required\r
Server: nginx/1.2.1\r
Date: Fri, 28 Jun 2013 17:11:04 GMT\r
Content-Type: text/html\r
Content-Length: 180\r
Connection: close\r
\r
<html>\r
<head><title>411 Length Required</title></head>\r
<body bgcolor="white">\r
<center><h1>411 Length Required</h1></center>\r
<hr><center>nginx/1.2.1</center>\r
</body>\r
</html>\r
I'd expect ra_serf to notice either the 411 or the Content-Type and
discard the body, but what happens is ra_serf attempts to parse the HTML
as XML.
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
Posted by Philip Martin <ph...@wandisco.com>.
Lieven Govaerts <sv...@mobsol.be> writes:
>> It seems to be connected to mod_deflate. My server was loading
>> mod_deflate (but not setting up any filters) and the import works with
>> the client setting
>>
>> http-compression = no
>>
>> but fails with
>>
>> http-compression = yes
>>
>> Still investigating.
>
> Don't you need r1497551?
I have that. If I configure nginx with "gzip = no" then lots of the
regression tests now pass when run through nginx with my patch.
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Lieven Govaerts <sv...@mobsol.be>.
On Fri, Jun 28, 2013 at 6:28 PM, Philip Martin
<ph...@wandisco.com> wrote:
> "Bert Huijben" <be...@qqmail.nl> writes:
>
>>> -----Original Message-----
>>> From: Philip Martin [mailto:philip.martin@wandisco.com]
>>> Sent: vrijdag 28 juni 2013 17:18
>>> To: Ivan Zhakov
>>> Cc: Greg Stein; dev@subversion.apache.org
>>> Subject: Re: svn commit: r1495419 - in
>>> /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c
>> util.c
>>>
>>> Philip Martin <ph...@wandisco.com> writes:
>>>
>>> > Suppose serf were to keep track of the number of outstanding requests
>>> > (it may already do that I haven't checked). Then if the number of
>>> > outstanding requests is zero when the 411 is received the downgrade to
>>> > HTTP/1.0 will be OK. Lots of client operations start with multiple
>>> > requests in serial before switching to pipelined requests, so in all
>>> > those cases the downgrade will work.
>>>
>>> I've been experimenting with the following patch to implement the above
>>> strategy. It's sufficient to allow me to checkout and commit through an
>>> nginx proxy that does not support chunked encoding. I had hoped to run
>>> the regression tests through nginx but for some reason the greek tree
>>> import fails, although the same import run manually outside the
>>> testsuite works. I'm not sure why that happens.
>>
>> Import starts with a few property read requests to obtain ignore and
>> autoprops. Perhaps one of these requests fails?
>
> It seems to be connected to mod_deflate. My server was loading
> mod_deflate (but not setting up any filters) and the import works with
> the client setting
>
> http-compression = no
>
> but fails with
>
> http-compression = yes
>
> Still investigating.
Don't you need r1497551?
> --
> Philip Martin | Subversion Committer
> WANdisco | Non-Stop Data
> www.wandisco.com
Pool memory bug in serf/ra_serf
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> It seems to be connected to mod_deflate. My server was loading
> mod_deflate (but not setting up any filters) and the import works with
> the client setting
>
> http-compression = no
>
> but fails with
>
> http-compression = yes
>
> Still investigating.
This appears to be a pool memory bug in serf or ra_serf. Setting
busted-proxy and then doing an import through nginx with
http-compression enabled gives:
==9035== Invalid read of size 4
==9035== at 0x8BFAA40: serf_bucket_mem_alloc (allocator.c:172)
==9035== by 0x8BFEDB4: serf_bucket_barrier_create (barrier_buckets.c:33)
==9035== by 0x6FDC6C3: accept_response (util.c:448)
==9035== by 0x6FDC70C: accept_head (util.c:463)
==9035== by 0x8BF5776: read_from_connection (outgoing.c:989)
==9035== by 0x8BF597C: serf__process_connection (outgoing.c:1108)
==9035== by 0x8BF37A2: serf_event_trigger (context.c:239)
==9035== by 0x8BF3915: serf_context_run (context.c:307)
==9035== by 0x6FDCE2B: svn_ra_serf__context_run_wait (util.c:736)
==9035== by 0x6FDD071: svn_ra_serf__context_run_one (util.c:810)
==9035== by 0x6FC4AC6: close_file (commit.c:2145)
==9035== by 0x4E6CA97: import_file (import.c:282)
==9035== by 0x4E6D79D: import (import.c:732)
==9035== by 0x4E6E5D4: svn_client_import5 (import.c:980)
==9035== by 0x412D5B: svn_cl__import (import-cmd.c:115)
==9035== by 0x4292A5: sub_main (svn.c:2929)
==9035== by 0x4295D2: main (svn.c:3020)
==9035== Address 0x9b27498 is 40 bytes inside a block of size 72 free'd
==9035== at 0x4C27D4E: free (vg_replace_malloc.c:427)
==9035== by 0x5F6D0E9: pool_clear_debug (apr_pools.c:1576)
==9035== by 0x5F6D23D: pool_destroy_debug (apr_pools.c:1638)
==9035== by 0x5F6D326: apr_pool_destroy_debug (apr_pools.c:1680)
==9035== by 0x5F6E69C: apr_pool_destroy (apr_pools.c:2610)
==9035== by 0x8BF46B9: destroy_request (outgoing.c:348)
==9035== by 0x8BF5860: read_from_connection (outgoing.c:1047)
==9035== by 0x8BF597C: serf__process_connection (outgoing.c:1108)
==9035== by 0x8BF37A2: serf_event_trigger (context.c:239)
==9035== by 0x8BF3915: serf_context_run (context.c:307)
==9035== by 0x6FDCE2B: svn_ra_serf__context_run_wait (util.c:736)
==9035== by 0x6FDD071: svn_ra_serf__context_run_one (util.c:810)
==9035== by 0x6FC42B2: add_file (commit.c:1923)
==9035== by 0x4E6C764: import_file (import.c:219)
==9035== by 0x4E6D79D: import (import.c:732)
==9035== by 0x4E6E5D4: svn_client_import5 (import.c:980)
==9035== by 0x412D5B: svn_cl__import (import-cmd.c:115)
==9035== by 0x4292A5: sub_main (svn.c:2929)
==9035== by 0x4295D2: main (svn.c:3020)
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
>> -----Original Message-----
>> From: Philip Martin [mailto:philip.martin@wandisco.com]
>> Sent: vrijdag 28 juni 2013 17:18
>> To: Ivan Zhakov
>> Cc: Greg Stein; dev@subversion.apache.org
>> Subject: Re: svn commit: r1495419 - in
>> /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c
> util.c
>>
>> Philip Martin <ph...@wandisco.com> writes:
>>
>> > Suppose serf were to keep track of the number of outstanding requests
>> > (it may already do that I haven't checked). Then if the number of
>> > outstanding requests is zero when the 411 is received the downgrade to
>> > HTTP/1.0 will be OK. Lots of client operations start with multiple
>> > requests in serial before switching to pipelined requests, so in all
>> > those cases the downgrade will work.
>>
>> I've been experimenting with the following patch to implement the above
>> strategy. It's sufficient to allow me to checkout and commit through an
>> nginx proxy that does not support chunked encoding. I had hoped to run
>> the regression tests through nginx but for some reason the greek tree
>> import fails, although the same import run manually outside the
>> testsuite works. I'm not sure why that happens.
>
> Import starts with a few property read requests to obtain ignore and
> autoprops. Perhaps one of these requests fails?
It seems to be connected to mod_deflate. My server was loading
mod_deflate (but not setting up any filters) and the import works with
the client setting
http-compression = no
but fails with
http-compression = yes
Still investigating.
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
RE: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: vrijdag 28 juni 2013 17:18
> To: Ivan Zhakov
> Cc: Greg Stein; dev@subversion.apache.org
> Subject: Re: svn commit: r1495419 - in
> /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c
util.c
>
> Philip Martin <ph...@wandisco.com> writes:
>
> > Suppose serf were to keep track of the number of outstanding requests
> > (it may already do that I haven't checked). Then if the number of
> > outstanding requests is zero when the 411 is received the downgrade to
> > HTTP/1.0 will be OK. Lots of client operations start with multiple
> > requests in serial before switching to pipelined requests, so in all
> > those cases the downgrade will work.
>
> I've been experimenting with the following patch to implement the above
> strategy. It's sufficient to allow me to checkout and commit through an
> nginx proxy that does not support chunked encoding. I had hoped to run
> the regression tests through nginx but for some reason the greek tree
> import fails, although the same import run manually outside the
> testsuite works. I'm not sure why that happens.
Import starts with a few property read requests to obtain ignore and
autoprops. Perhaps one of these requests fails?
Bert
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> Could you try svn trunk? Set busted-proxy=yes in your config to enable
> the new behavior.
>
> Without the knob turned out, you should get a 411 error. As Stefan
> pointed out, we may want to consider detecting 411 and providing a
> better error message.
>
> With the knob enabled, it should auto-detect your nginx proxy and
> stick to using C-L requests.
I thought I used this successfully yesterday but think I was mistaken;
it certainly doesn't work for me today. I see the extra OPTIONS request
being sent and the 411 being received but the 411 handling is missed
because it's inside an "if (err)" and err is SVN_NO_ERROR. Has anyone
used this code? I need this patch:
Index: subversion/libsvn_ra_serf/options.c
===================================================================
--- subversion/libsvn_ra_serf/options.c (revision 1499379)
+++ subversion/libsvn_ra_serf/options.c (working copy)
@@ -545,7 +545,6 @@ svn_ra_serf__probe_proxy(svn_ra_serf__session_t *s
apr_pool_t *scratch_pool)
{
svn_ra_serf__handler_t *handler;
- svn_error_t *err;
handler = apr_pcalloc(scratch_pool, sizeof(*handler));
handler->handler_pool = scratch_pool;
@@ -562,27 +561,19 @@ svn_ra_serf__probe_proxy(svn_ra_serf__session_t *s
/* No special headers. */
- err = svn_ra_serf__context_run_one(handler, scratch_pool);
- if (err)
+ SVN_ERR(svn_ra_serf__context_run_one(handler, scratch_pool));
+ /* Some versions of nginx in reverse proxy mode will return 411. They want
+ a Content-Length header, rather than chunked requests. We can keep other
+ HTTP/1.1 features, but will disable the chunking. */
+ if (handler->sline.code == 411)
{
- /* Some versions of nginx in reverse proxy mode will return 411. They want
- a Content-Length header, rather than chunked requests. We can keep other
- HTTP/1.1 features, but will disable the chunking. */
- if (handler->sline.code == 411)
- {
- serf_sess->using_chunked_requests = FALSE;
+ serf_sess->using_chunked_requests = FALSE;
- svn_error_clear(err);
- return SVN_NO_ERROR;
- }
-
- return svn_error_trace(
- svn_error_compose_create(
- svn_ra_serf__error_on_status(handler->sline,
- serf_sess->session_url.path,
- handler->location),
- err));
+ return SVN_NO_ERROR;
}
+ SVN_ERR(svn_ra_serf__error_on_status(handler->sline,
+ handler->path,
+ handler->location));
return SVN_NO_ERROR;
}
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Jun 28, 2013 at 1:30 PM, Greg Stein <gs...@gmail.com> wrote:
>
>
> On Jun 28, 2013 11:17 AM, "Philip Martin" <ph...@wandisco.com> wrote:
> >
> > Philip Martin <ph...@wandisco.com> writes:
> >
> > > Suppose serf were to keep track of the number of outstanding requests
> > > (it may already do that I haven't checked). Then if the number of
> > > outstanding requests is zero when the 411 is received the downgrade to
> > > HTTP/1.0 will be OK. Lots of client operations start with multiple
> > > requests in serial before switching to pipelined requests, so in all
> > > those cases the downgrade will work.
> >
> > I've been experimenting with the following patch to implement the above
> > strategy. It's sufficient to allow me to checkout and commit through an
> > nginx proxy that does not support chunked encoding. I had hoped to run
> > the regression tests through nginx but for some reason the greek tree
> > import fails, although the same import run manually outside the
> > testsuite works. I'm not sure why that happens.
>
> Not everything uses run_one(). It is easier to send a second OPTIONS than to catch the next request, wherever that may be. (and it could be very hard to replay)
Could you try svn trunk? Set busted-proxy=yes in your config to enable
the new behavior.
Without the knob turned out, you should get a 411 error. As Stefan
pointed out, we may want to consider detecting 411 and providing a
better error message.
With the knob enabled, it should auto-detect your nginx proxy and
stick to using C-L requests.
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Jun 28, 2013 11:17 AM, "Philip Martin" <ph...@wandisco.com>
wrote:
>
> Philip Martin <ph...@wandisco.com> writes:
>
> > Suppose serf were to keep track of the number of outstanding requests
> > (it may already do that I haven't checked). Then if the number of
> > outstanding requests is zero when the 411 is received the downgrade to
> > HTTP/1.0 will be OK. Lots of client operations start with multiple
> > requests in serial before switching to pipelined requests, so in all
> > those cases the downgrade will work.
>
> I've been experimenting with the following patch to implement the above
> strategy. It's sufficient to allow me to checkout and commit through an
> nginx proxy that does not support chunked encoding. I had hoped to run
> the regression tests through nginx but for some reason the greek tree
> import fails, although the same import run manually outside the
> testsuite works. I'm not sure why that happens.
Not everything uses run_one(). It is easier to send a second OPTIONS than
to catch the next request, wherever that may be. (and it could be very hard
to replay)
Cheers,
-g
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Suppose serf were to keep track of the number of outstanding requests
> (it may already do that I haven't checked). Then if the number of
> outstanding requests is zero when the 411 is received the downgrade to
> HTTP/1.0 will be OK. Lots of client operations start with multiple
> requests in serial before switching to pipelined requests, so in all
> those cases the downgrade will work.
I've been experimenting with the following patch to implement the above
strategy. It's sufficient to allow me to checkout and commit through an
nginx proxy that does not support chunked encoding. I had hoped to run
the regression tests through nginx but for some reason the greek tree
import fails, although the same import run manually outside the
testsuite works. I'm not sure why that happens.
Index: subversion/libsvn_ra_serf/options.c
===================================================================
--- subversion/libsvn_ra_serf/options.c (revision 1497686)
+++ subversion/libsvn_ra_serf/options.c (working copy)
@@ -380,7 +380,14 @@ options_response_handler(serf_request_t *request,
return opt_ctx->inner_handler(request, response, opt_ctx->inner_baton, pool);
}
+static void
+request_reset(void *reset_baton)
+{
+ options_context_t *opt_ctx = reset_baton;
+ opt_ctx->headers_processed = FALSE;
+}
+
static svn_error_t *
create_options_req(options_context_t **opt_ctx,
svn_ra_serf__session_t *session,
@@ -417,6 +424,8 @@ create_options_req(options_context_t **opt_ctx,
new_ctx->inner_baton = handler->response_baton;
handler->response_handler = options_response_handler;
handler->response_baton = new_ctx;
+ handler->reset = request_reset;
+ handler->reset_baton = new_ctx;
*opt_ctx = new_ctx;
Index: subversion/libsvn_ra_serf/ra_serf.h
===================================================================
--- subversion/libsvn_ra_serf/ra_serf.h (revision 1497686)
+++ subversion/libsvn_ra_serf/ra_serf.h (working copy)
@@ -147,6 +147,8 @@ struct svn_ra_serf__session_t {
/* Should we use Transfer-Encoding: chunked for HTTP/1.1 servers. */
svn_boolean_t using_chunked_requests;
+ int requests_outstanding;
+
/* Our Version-Controlled-Configuration; may be NULL until we know it. */
const char *vcc_url;
@@ -410,6 +412,9 @@ typedef svn_error_t *
int status_code,
void *baton);
+typedef void
+(*svn_ra_serf__request_reset_t)(void *baton);
+
/* ### we should reorder the types in this file. */
typedef struct svn_ra_serf__server_error_t svn_ra_serf__server_error_t;
@@ -478,10 +483,15 @@ typedef struct svn_ra_serf__handler_t {
svn_ra_serf__request_body_delegate_t body_delegate;
void *body_delegate_baton;
+ svn_ra_serf__request_reset_t reset;
+ void *reset_baton;
+
/* The connection and session to be used for this request. */
svn_ra_serf__connection_t *conn;
svn_ra_serf__session_t *session;
+ svn_boolean_t response_seen;
+
/* Internal flag to indicate we've parsed the headers. */
svn_boolean_t reading_body;
Index: subversion/libsvn_ra_serf/util.c
===================================================================
--- subversion/libsvn_ra_serf/util.c (revision 1497686)
+++ subversion/libsvn_ra_serf/util.c (working copy)
@@ -800,7 +800,9 @@ svn_ra_serf__context_run_one(svn_ra_serf__handler_
apr_pool_t *scratch_pool)
{
svn_error_t *err;
+ svn_boolean_t first_time = TRUE;
+ retry:
/* Create a serf request based on HANDLER. */
svn_ra_serf__request_create(handler);
@@ -813,6 +815,17 @@ svn_ra_serf__context_run_one(svn_ra_serf__handler_
handler->server_error = NULL;
}
+ if (handler->sline.code == 411 && first_time)
+ {
+ handler->session->using_chunked_requests = FALSE;
+
+ if (!err && !handler->session->requests_outstanding)
+ {
+ first_time = FALSE;
+ goto retry;
+ }
+ }
+
return svn_error_trace(err);
}
@@ -1826,6 +1839,12 @@ handle_response(serf_request_t *request,
### ignored by the caller. */
*serf_status = APR_SUCCESS;
+ if (!handler->response_seen)
+ {
+ handler->response_seen = TRUE;
+ --handler->session->requests_outstanding;
+ }
+
if (!response)
{
/* Uh-oh. Our connection died. */
@@ -2207,8 +2226,12 @@ svn_ra_serf__request_create(svn_ra_serf__handler_t
handler->server_error = NULL;
handler->sline.version = 0;
handler->location = NULL;
+ handler->response_seen = FALSE;
handler->reading_body = FALSE;
handler->discard_body = FALSE;
+ ++handler->session->requests_outstanding;
+ if (handler->reset)
+ handler->reset(handler->reset_baton);
/* ### do we ever alter the >response_handler? */
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> Problem with this approach that some servers may support HTTP/1.1
> partially. I.e. declare them as HTTP/1.1 but do not support chunked
> Transfer-Encoding.
>
> I wanted to avoid downgrade to HTTP/1.0 on later requests because it
> could introduce requests ordering issue. For example:
> C: HTTP/1.0 OPTIONS /
> S: HTTP/1.1 Response
>
> C: HTTP/1.1 PROPFIND
> C: HTTP/1.1 GET
> S: HTTP/1.1 411 Length required
> S: HTTP/1.0 200 OK
>
> Then client send PROPFIND request again:
> C: HTTP/1.0 PROPFIND
> S: HTTP/1.1 200 OK
>
> But requests was completed and different order than our update editor
> expected and most likely crash :(
Suppose serf were to keep track of the number of outstanding requests
(it may already do that I haven't checked). Then if the number of
outstanding requests is zero when the 411 is received the downgrade to
HTTP/1.0 will be OK. Lots of client operations start with multiple
requests in serial before switching to pipelined requests, so in all
those cases the downgrade will work.
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
> On Fri, Jun 21, 2013 at 9:19 AM, Greg Stein <gs...@gmail.com> wrote:
>> Please revert this. You *cannot* rely on a 411 response.
>>
>> You are also not allowed to send a 1.1 request to a server for which
>> you don't know if they support 1.0 vs 1.1.
>>
On Fri, Jun 21, 2013 at 5:28 PM, Greg Stein <gs...@gmail.com> wrote:
> This has been quoted before, but I'll repeat it again. RFC 2145, Section 2:
>
> "One consequence of these rules is that an HTTP/1.1 message sent to an
> HTTP/1.0 recipient (or a recipient whose version is unknown) MUST be
> constructed so that it remains a valid HTTP/1.0 message when all
> headers not defined in the HTTP/1.0 specification [1] are removed."
>
> Removing the Transfer-Coding: chunked header completely alters the
> message. Thus, it cannot be sent to a 1.0 server. This is why you
> cannot rely on any particular behavior or response status.
Hi Greg! Thanks for review.
My patch doesn't rely on 411 response only: ra_serf will downgrade to
HTTP/1.0 if *any* HTTP/1.0 response received *or* HTTP/1.1 411 status
code.
I've tested several browsers and they use HTTP/1.1 as first request.
>> Sigh. Seriously, man. This is completely counter to the work that was
>> done. We *start* with 1.0, and then we upgrade to 1.1 only when the
>> server identifies that it can handle it. Not to mention the
>> complications of intervening proxies. You're making a mess of that
>> work.
>>
Problem with this approach that some servers may support HTTP/1.1
partially. I.e. declare them as HTTP/1.1 but do not support chunked
Transfer-Encoding.
I wanted to avoid downgrade to HTTP/1.0 on later requests because it
could introduce requests ordering issue. For example:
C: HTTP/1.0 OPTIONS /
S: HTTP/1.1 Response
C: HTTP/1.1 PROPFIND
C: HTTP/1.1 GET
S: HTTP/1.1 411 Length required
S: HTTP/1.0 200 OK
Then client send PROPFIND request again:
C: HTTP/1.0 PROPFIND
S: HTTP/1.1 200 OK
But requests was completed and different order than our update editor
expected and most likely crash :(
--
Ivan Zhakov
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
This has been quoted before, but I'll repeat it again. RFC 2145, Section 2:
"One consequence of these rules is that an HTTP/1.1 message sent to an
HTTP/1.0 recipient (or a recipient whose version is unknown) MUST be
constructed so that it remains a valid HTTP/1.0 message when all
headers not defined in the HTTP/1.0 specification [1] are removed."
Removing the Transfer-Coding: chunked header completely alters the
message. Thus, it cannot be sent to a 1.0 server. This is why you
cannot rely on any particular behavior or response status.
-g
On Fri, Jun 21, 2013 at 9:19 AM, Greg Stein <gs...@gmail.com> wrote:
> Please revert this. You *cannot* rely on a 411 response.
>
> You are also not allowed to send a 1.1 request to a server for which
> you don't know if they support 1.0 vs 1.1.
>
> Sigh. Seriously, man. This is completely counter to the work that was
> done. We *start* with 1.0, and then we upgrade to 1.1 only when the
> server identifies that it can handle it. Not to mention the
> complications of intervening proxies. You're making a mess of that
> work.
>
> -g
>
> On Fri, Jun 21, 2013 at 8:47 AM, <iv...@apache.org> wrote:
>> Author: ivan
>> Date: Fri Jun 21 12:47:46 2013
>> New Revision: 1495419
>>
>> URL: http://svn.apache.org/r1495419
>> Log:
>> Rework server chunked transfer encoding support detection in ra_serf. Use
>> HTTP/1.1 chunked transfer encoding for first OPTIONS request and then
>> fallback to HTTP/1.0 requests if HTTP 411 status code or HTTP/1.0 response
>> received.
>>
>> Discussion and report: http://svn.haxx.se/dev/archive-2013-06/0408.shtml
>>
>> * subversion/libsvn_ra_serf/ra_serf.h
>> (svn_ra_serf__session_t): Remove HTTP10 and add USE_CHUNKED_ENCODING.
>> * subversion/libsvn_ra_serf/options.c
>> (svn_ra_serf__exchange_capabilities): Retry OPTIONS request without using
>> chunked transfer encoding if HTTP 411 status code or HTTP/1.0 response
>> received for chunked request.
>> * subversion/libsvn_ra_serf/serf.c
>> (svn_ra_serf__open): Initialize USE_CHUNKED_ENCODING to TRUE.
>> * subversion/libsvn_ra_serf/util.c
>> (setup_serf_req): Use USE_CHUNKED_ENCODING member instead of removed
>> HTTP10. Always send 'Connection: keep-alive' header.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_ra_serf/options.c
>> subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
>> subversion/trunk/subversion/libsvn_ra_serf/serf.c
>> subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>> Modified: subversion/trunk/subversion/libsvn_ra_serf/options.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/options.c?rev=1495419&r1=1495418&r2=1495419&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_ra_serf/options.c (original)
>> +++ subversion/trunk/subversion/libsvn_ra_serf/options.c Fri Jun 21 12:47:46 2013
>> @@ -488,6 +488,22 @@ svn_ra_serf__exchange_capabilities(svn_r
>>
>> err = svn_ra_serf__context_run_one(opt_ctx->handler, pool);
>>
>> + /* Retry request if HTTP/1.1 411 Length Required or we got HTTP/1.0 response. */
>> + if (serf_sess->use_chunked_encoding &&
>> + (opt_ctx->handler->sline.code == 411 ||
>> + opt_ctx->handler->sline.version == SERF_HTTP_10))
>> + {
>> + /* Ignore any errors and retry request using HTTP/1.0 with
>> + Content-Length.*/
>> + svn_error_clear(err);
>> +
>> + serf_sess->use_chunked_encoding = FALSE;
>> +
>> + SVN_ERR(create_options_req(&opt_ctx, serf_sess, serf_sess->conns[0], pool));
>> +
>> + err = svn_ra_serf__context_run_one(opt_ctx->handler, pool);
>> + }
>> +
>> /* If our caller cares about server redirections, and our response
>> carries such a thing, report as much. We'll disregard ERR --
>> it's most likely just a complaint about the response body not
>>
>> Modified: subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h?rev=1495419&r1=1495418&r2=1495419&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h (original)
>> +++ subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h Fri Jun 21 12:47:46 2013
>> @@ -140,9 +140,8 @@ struct svn_ra_serf__session_t {
>> apr_uri_t repos_root;
>> const char *repos_root_str;
>>
>> - /* The server is not Apache/mod_dav_svn (directly) and only supports
>> - HTTP/1.0. Thus, we cannot send chunked requests. */
>> - svn_boolean_t http10;
>> + /* The server supports chunked request bodies. */
>> + svn_boolean_t use_chunked_encoding;
>>
>> /* Our Version-Controlled-Configuration; may be NULL until we know it. */
>> const char *vcc_url;
>>
>> Modified: subversion/trunk/subversion/libsvn_ra_serf/serf.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/serf.c?rev=1495419&r1=1495418&r2=1495419&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_ra_serf/serf.c (original)
>> +++ subversion/trunk/subversion/libsvn_ra_serf/serf.c Fri Jun 21 12:47:46 2013
>> @@ -437,9 +437,10 @@ svn_ra_serf__open(svn_ra_session_t *sess
>>
>> serf_sess->capabilities = apr_hash_make(serf_sess->pool);
>>
>> - /* We have to assume that the server only supports HTTP/1.0. Once it's clear
>> - HTTP/1.1 is supported, we can upgrade. */
>> - serf_sess->http10 = TRUE;
>> + /* Assume HTTP/1.1 server and use chunked transfer encoding. We fallback
>> + * to HTTP/1.0 requests in svn_ra_serf__exchange_capabilities() if server
>> + * doesn't support chunked encoding. */
>> + serf_sess->use_chunked_encoding = FALSE;
>>
>> SVN_ERR(load_config(serf_sess, config, serf_sess->pool));
>>
>>
>> Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1495419&r1=1495418&r2=1495419&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
>> +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Fri Jun 21 12:47:46 2013
>> @@ -642,7 +642,7 @@ setup_serf_req(serf_request_t *request,
>>
>> svn_spillbuf_t *buf;
>>
>> - if (session->http10 && body_bkt != NULL)
>> + if (!session->use_chunked_encoding && body_bkt != NULL)
>> {
>> /* Ugh. Use HTTP/1.0 to talk to the server because we don't know if
>> it speaks HTTP/1.1 (and thus, chunked requests), or because the
>> @@ -670,7 +670,7 @@ setup_serf_req(serf_request_t *request,
>>
>> /* Set the Content-Length value. This will also trigger an HTTP/1.0
>> request (rather than the default chunked request). */
>> - if (session->http10)
>> + if (!session->use_chunked_encoding)
>> {
>> if (body_bkt == NULL)
>> serf_bucket_request_set_CL(*req_bkt, 0);
>> @@ -690,10 +690,9 @@ setup_serf_req(serf_request_t *request,
>> serf_bucket_headers_setn(*hdrs_bkt, "Content-Type", content_type);
>> }
>>
>> - if (session->http10)
>> - {
>> - serf_bucket_headers_setn(*hdrs_bkt, "Connection", "keep-alive");
>> - }
>> + /* Always set Connection: keep-alive because we don't know if server
>> + * is HTTP/1.1 aware. */
>> + serf_bucket_headers_setn(*hdrs_bkt, "Connection", "keep-alive");
>>
>> if (accept_encoding)
>> {
>> @@ -1861,10 +1860,6 @@ handle_response(serf_request_t *request,
>>
>> handler->sline = sl;
>> handler->sline.reason = apr_pstrdup(handler->handler_pool, sl.reason);
>> -
>> - /* HTTP/1.1? (or later) */
>> - if (sl.version != SERF_HTTP_10)
>> - handler->session->http10 = FALSE;
>> }
>>
>> /* Keep reading from the network until we've read all the headers. */
>>
>>
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf:
options.c ra_serf.h serf.c util.c
Posted by Greg Stein <gs...@gmail.com>.
Please revert this. You *cannot* rely on a 411 response.
You are also not allowed to send a 1.1 request to a server for which
you don't know if they support 1.0 vs 1.1.
Sigh. Seriously, man. This is completely counter to the work that was
done. We *start* with 1.0, and then we upgrade to 1.1 only when the
server identifies that it can handle it. Not to mention the
complications of intervening proxies. You're making a mess of that
work.
-g
On Fri, Jun 21, 2013 at 8:47 AM, <iv...@apache.org> wrote:
> Author: ivan
> Date: Fri Jun 21 12:47:46 2013
> New Revision: 1495419
>
> URL: http://svn.apache.org/r1495419
> Log:
> Rework server chunked transfer encoding support detection in ra_serf. Use
> HTTP/1.1 chunked transfer encoding for first OPTIONS request and then
> fallback to HTTP/1.0 requests if HTTP 411 status code or HTTP/1.0 response
> received.
>
> Discussion and report: http://svn.haxx.se/dev/archive-2013-06/0408.shtml
>
> * subversion/libsvn_ra_serf/ra_serf.h
> (svn_ra_serf__session_t): Remove HTTP10 and add USE_CHUNKED_ENCODING.
> * subversion/libsvn_ra_serf/options.c
> (svn_ra_serf__exchange_capabilities): Retry OPTIONS request without using
> chunked transfer encoding if HTTP 411 status code or HTTP/1.0 response
> received for chunked request.
> * subversion/libsvn_ra_serf/serf.c
> (svn_ra_serf__open): Initialize USE_CHUNKED_ENCODING to TRUE.
> * subversion/libsvn_ra_serf/util.c
> (setup_serf_req): Use USE_CHUNKED_ENCODING member instead of removed
> HTTP10. Always send 'Connection: keep-alive' header.
>
> Modified:
> subversion/trunk/subversion/libsvn_ra_serf/options.c
> subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
> subversion/trunk/subversion/libsvn_ra_serf/serf.c
> subversion/trunk/subversion/libsvn_ra_serf/util.c
>
> Modified: subversion/trunk/subversion/libsvn_ra_serf/options.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/options.c?rev=1495419&r1=1495418&r2=1495419&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_serf/options.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/options.c Fri Jun 21 12:47:46 2013
> @@ -488,6 +488,22 @@ svn_ra_serf__exchange_capabilities(svn_r
>
> err = svn_ra_serf__context_run_one(opt_ctx->handler, pool);
>
> + /* Retry request if HTTP/1.1 411 Length Required or we got HTTP/1.0 response. */
> + if (serf_sess->use_chunked_encoding &&
> + (opt_ctx->handler->sline.code == 411 ||
> + opt_ctx->handler->sline.version == SERF_HTTP_10))
> + {
> + /* Ignore any errors and retry request using HTTP/1.0 with
> + Content-Length.*/
> + svn_error_clear(err);
> +
> + serf_sess->use_chunked_encoding = FALSE;
> +
> + SVN_ERR(create_options_req(&opt_ctx, serf_sess, serf_sess->conns[0], pool));
> +
> + err = svn_ra_serf__context_run_one(opt_ctx->handler, pool);
> + }
> +
> /* If our caller cares about server redirections, and our response
> carries such a thing, report as much. We'll disregard ERR --
> it's most likely just a complaint about the response body not
>
> Modified: subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h?rev=1495419&r1=1495418&r2=1495419&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h Fri Jun 21 12:47:46 2013
> @@ -140,9 +140,8 @@ struct svn_ra_serf__session_t {
> apr_uri_t repos_root;
> const char *repos_root_str;
>
> - /* The server is not Apache/mod_dav_svn (directly) and only supports
> - HTTP/1.0. Thus, we cannot send chunked requests. */
> - svn_boolean_t http10;
> + /* The server supports chunked request bodies. */
> + svn_boolean_t use_chunked_encoding;
>
> /* Our Version-Controlled-Configuration; may be NULL until we know it. */
> const char *vcc_url;
>
> Modified: subversion/trunk/subversion/libsvn_ra_serf/serf.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/serf.c?rev=1495419&r1=1495418&r2=1495419&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_serf/serf.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/serf.c Fri Jun 21 12:47:46 2013
> @@ -437,9 +437,10 @@ svn_ra_serf__open(svn_ra_session_t *sess
>
> serf_sess->capabilities = apr_hash_make(serf_sess->pool);
>
> - /* We have to assume that the server only supports HTTP/1.0. Once it's clear
> - HTTP/1.1 is supported, we can upgrade. */
> - serf_sess->http10 = TRUE;
> + /* Assume HTTP/1.1 server and use chunked transfer encoding. We fallback
> + * to HTTP/1.0 requests in svn_ra_serf__exchange_capabilities() if server
> + * doesn't support chunked encoding. */
> + serf_sess->use_chunked_encoding = FALSE;
>
> SVN_ERR(load_config(serf_sess, config, serf_sess->pool));
>
>
> Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1495419&r1=1495418&r2=1495419&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Fri Jun 21 12:47:46 2013
> @@ -642,7 +642,7 @@ setup_serf_req(serf_request_t *request,
>
> svn_spillbuf_t *buf;
>
> - if (session->http10 && body_bkt != NULL)
> + if (!session->use_chunked_encoding && body_bkt != NULL)
> {
> /* Ugh. Use HTTP/1.0 to talk to the server because we don't know if
> it speaks HTTP/1.1 (and thus, chunked requests), or because the
> @@ -670,7 +670,7 @@ setup_serf_req(serf_request_t *request,
>
> /* Set the Content-Length value. This will also trigger an HTTP/1.0
> request (rather than the default chunked request). */
> - if (session->http10)
> + if (!session->use_chunked_encoding)
> {
> if (body_bkt == NULL)
> serf_bucket_request_set_CL(*req_bkt, 0);
> @@ -690,10 +690,9 @@ setup_serf_req(serf_request_t *request,
> serf_bucket_headers_setn(*hdrs_bkt, "Content-Type", content_type);
> }
>
> - if (session->http10)
> - {
> - serf_bucket_headers_setn(*hdrs_bkt, "Connection", "keep-alive");
> - }
> + /* Always set Connection: keep-alive because we don't know if server
> + * is HTTP/1.1 aware. */
> + serf_bucket_headers_setn(*hdrs_bkt, "Connection", "keep-alive");
>
> if (accept_encoding)
> {
> @@ -1861,10 +1860,6 @@ handle_response(serf_request_t *request,
>
> handler->sline = sl;
> handler->sline.reason = apr_pstrdup(handler->handler_pool, sl.reason);
> -
> - /* HTTP/1.1? (or later) */
> - if (sl.version != SERF_HTTP_10)
> - handler->session->http10 = FALSE;
> }
>
> /* Keep reading from the network until we've read all the headers. */
>
>