You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2015/11/19 19:03:12 UTC
svn commit: r1715228 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Author: rhuijben
Date: Thu Nov 19 18:03:11 2015
New Revision: 1715228
URL: http://svn.apache.org/viewvc?rev=1715228&view=rev
Log:
Add a tiny bit of code to allow testing with Apache Serf's http/2 support.
I committed this patch to celebrate that I got through basic_tests.py
using the current http/2 support.
* subversion/libsvn_ra_serf/util.c
(conn_negotiate_protocol): New function.
(conn_setup): Register usage of conn_negotiate_protocol when
a very recent (read: trunk) serf + SVN__SERF_TEST_HTTP2 are used.
Modified:
subversion/trunk/subversion/libsvn_ra_serf/util.c
Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1715228&r1=1715227&r2=1715228&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/util.c Thu Nov 19 18:03:11 2015
@@ -480,6 +480,36 @@ load_authorities(svn_ra_serf__connection
return SVN_NO_ERROR;
}
+#if SERF_VERSION_AT_LEAST(1, 4, 0) && defined(SVN__SERF_TEST_HTTP2)
+/* Implements serf_ssl_protocol_result_cb_t */
+static apr_status_t
+conn_negotiate_protocol(void *data,
+ const char *protocol)
+{
+ svn_ra_serf__connection_t *conn = data;
+
+ if (!strcmp(protocol, "h2"))
+ {
+ serf_connection_set_framing_type(
+ conn->conn,
+ SERF_CONNECTION_FRAMING_TYPE_HTTP2);
+
+ /* Disable generating content-length headers. */
+ conn->session->http10 = FALSE;
+ conn->session->using_chunked_requests = TRUE;
+ }
+ else
+ {
+ /* protocol should be "" or "http/1.1" */
+ serf_connection_set_framing_type(
+ conn->conn,
+ SERF_CONNECTION_FRAMING_TYPE_HTTP1);
+ }
+
+ return APR_SUCCESS;
+}
+#endif
+
static svn_error_t *
conn_setup(apr_socket_t *sock,
serf_bucket_t **read_bkt,
@@ -525,6 +555,16 @@ conn_setup(apr_socket_t *sock,
SVN_ERR(load_authorities(conn, conn->session->ssl_authorities,
conn->session->pool));
}
+#if SERF_VERSION_AT_LEAST(1, 4, 0) && defined(SVN__SERF_TEST_HTTP2)
+ if (APR_SUCCESS ==
+ serf_ssl_negotiate_protocol(conn->ssl_context, "h2,http/1.1",
+ conn_negotiate_protocol, conn))
+ {
+ serf_connection_set_framing_type(
+ conn->conn,
+ SERF_CONNECTION_FRAMING_TYPE_NONE);
+ }
+#endif
}
if (write_bkt)
RE: svn commit:
r1715228-/subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
I think it is possible to handle the symptoms properly in serf via a combination of priority+windowing. The receiver should just allow storing the first window of data before it processes the rest.
Our current spillbuffer should handle this properly, as this is exactly what we do today with an update report that wants to fetch too many files.
Bert
Sent from Outlook Mail for Windows 10 phone
From: Ivan Zhakov
Sent: vrijdag 20 november 2015 10:27
To: Bert Huijben
Cc: dev@subversion.apache.org
Subject: Re: svn commit: r1715228-/subversion/trunk/subversion/libsvn_ra_serf/util.c
On 20 November 2015 at 12:18, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> Sent: vrijdag 20 november 2015 10:12
>> To: Bert Huijben <be...@qqmail.nl>
>> Cc: dev@subversion.apache.org; dev@serf.apache.org
>> Subject: Re: svn commit: r1715228 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>> On 20 November 2015 at 11:53, Bert Huijben <be...@qqmail.nl> wrote:
>> > Ack.
>> >
>> > Not pipelining, or not sending simultaneous/parallel requests if you don’t
>> > want to depend on the order in which they are received is the safest thing.
>> >
>> > And the current code can break even in http/1. Svnrdump even has special
>> > precautions for this even though I have no idea why it would see the
>> > problems it describes it has. (The editor drive is 100% from one http
>> > response)
>> >
>> > What we could do is replace the current assertion with a request on a
>> > completely different connection to retrieve the properties if we don’t have
>> > them in time… as a fallback mechanism to at least continue going. This also
>> > works for legacy servers if the first improvement is applied.
>> >
>> > I’m not sure if the current implementation has more problems… E.g. if
>> > revisions can also be received out of order.
>> >
>> > Going to a single report –that includes the revprops- for multiple revisions
>> > is a safe extension, that will improve in all cases: HTTP and HTTP/2 alike.
>> >
>> I understand that current is also unsafe, that's why I suggest go
>> single REPORT and disable pipeling for older servers. I'll add this to
>> my TODO list you agree that this approach makes sense.
>
> +1.
>
Ok, I'll try implement it. But I want release Subversion 1.9.3 first.
> This would solve +- 50% of the current testfailures running over http/2.
>
> But we should implement a fix that works for old servers too. I will work on that part.
>
Yes, but I think disable pipelining is also viable option if we
implement replay-range REPORT.
>
> At least some other failures I see are caused by getting httpd temporarily
> in a 100% CPU loop, which causes other tests that run at the same time to
> fail. My current best guess is that this is a server side issue, but I'll have to
> investigate. But not in the daytime hours of the next few days.
>
>
> Note that the easiest way not to pipeline is: not scheduling requests that you are not able to handle yet :)
>
> Bert
>
--
Ivan Zhakov
Re: svn commit: r1715228 -/subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 20 November 2015 at 12:18, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> Sent: vrijdag 20 november 2015 10:12
>> To: Bert Huijben <be...@qqmail.nl>
>> Cc: dev@subversion.apache.org; dev@serf.apache.org
>> Subject: Re: svn commit: r1715228 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>> On 20 November 2015 at 11:53, Bert Huijben <be...@qqmail.nl> wrote:
>> > Ack.
>> >
>> > Not pipelining, or not sending simultaneous/parallel requests if you don’t
>> > want to depend on the order in which they are received is the safest thing.
>> >
>> > And the current code can break even in http/1. Svnrdump even has special
>> > precautions for this even though I have no idea why it would see the
>> > problems it describes it has. (The editor drive is 100% from one http
>> > response)
>> >
>> > What we could do is replace the current assertion with a request on a
>> > completely different connection to retrieve the properties if we don’t have
>> > them in time… as a fallback mechanism to at least continue going. This also
>> > works for legacy servers if the first improvement is applied.
>> >
>> > I’m not sure if the current implementation has more problems… E.g. if
>> > revisions can also be received out of order.
>> >
>> > Going to a single report –that includes the revprops- for multiple revisions
>> > is a safe extension, that will improve in all cases: HTTP and HTTP/2 alike.
>> >
>> I understand that current is also unsafe, that's why I suggest go
>> single REPORT and disable pipeling for older servers. I'll add this to
>> my TODO list you agree that this approach makes sense.
>
> +1.
>
Ok, I'll try implement it. But I want release Subversion 1.9.3 first.
> This would solve +- 50% of the current testfailures running over http/2.
>
> But we should implement a fix that works for old servers too. I will work on that part.
>
Yes, but I think disable pipelining is also viable option if we
implement replay-range REPORT.
>
> At least some other failures I see are caused by getting httpd temporarily
> in a 100% CPU loop, which causes other tests that run at the same time to
> fail. My current best guess is that this is a server side issue, but I'll have to
> investigate. But not in the daytime hours of the next few days.
>
>
> Note that the easiest way not to pipeline is: not scheduling requests that you are not able to handle yet :)
>
> Bert
>
--
Ivan Zhakov
RE: svn commit: r1715228 -/subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: vrijdag 20 november 2015 10:12
> To: Bert Huijben <be...@qqmail.nl>
> Cc: dev@subversion.apache.org; dev@serf.apache.org
> Subject: Re: svn commit: r1715228 -
> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>
> On 20 November 2015 at 11:53, Bert Huijben <be...@qqmail.nl> wrote:
> > Ack.
> >
> > Not pipelining, or not sending simultaneous/parallel requests if you don’t
> > want to depend on the order in which they are received is the safest thing.
> >
> > And the current code can break even in http/1. Svnrdump even has special
> > precautions for this even though I have no idea why it would see the
> > problems it describes it has. (The editor drive is 100% from one http
> > response)
> >
> > What we could do is replace the current assertion with a request on a
> > completely different connection to retrieve the properties if we don’t have
> > them in time… as a fallback mechanism to at least continue going. This also
> > works for legacy servers if the first improvement is applied.
> >
> > I’m not sure if the current implementation has more problems… E.g. if
> > revisions can also be received out of order.
> >
> > Going to a single report –that includes the revprops- for multiple revisions
> > is a safe extension, that will improve in all cases: HTTP and HTTP/2 alike.
> >
> I understand that current is also unsafe, that's why I suggest go
> single REPORT and disable pipeling for older servers. I'll add this to
> my TODO list you agree that this approach makes sense.
+1.
This would solve +- 50% of the current testfailures running over http/2.
But we should implement a fix that works for old servers too. I will work on that part.
At least some other failures I see are caused by getting httpd temporarily in a 100% CPU loop, which causes other tests that run at the same time to fail. My current best guess is that this is a server side issue, but I'll have to investigate. But not in the daytime hours of the next few days.
Note that the easiest way not to pipeline is: not scheduling requests that you are not able to handle yet :)
Bert
Re: svn commit: r1715228 -/subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 20 November 2015 at 11:53, Bert Huijben <be...@qqmail.nl> wrote:
> Ack.
>
> Not pipelining, or not sending simultaneous/parallel requests if you don’t
> want to depend on the order in which they are received is the safest thing.
>
> And the current code can break even in http/1. Svnrdump even has special
> precautions for this even though I have no idea why it would see the
> problems it describes it has. (The editor drive is 100% from one http
> response)
>
> What we could do is replace the current assertion with a request on a
> completely different connection to retrieve the properties if we don’t have
> them in time… as a fallback mechanism to at least continue going. This also
> works for legacy servers if the first improvement is applied.
>
> I’m not sure if the current implementation has more problems… E.g. if
> revisions can also be received out of order.
>
> Going to a single report –that includes the revprops- for multiple revisions
> is a safe extension, that will improve in all cases: HTTP and HTTP/2 alike.
>
I understand that current is also unsafe, that's why I suggest go
single REPORT and disable pipeling for older servers. I'll add this to
my TODO list you agree that this approach makes sense.
> I will start looking in supporting priority scheduling anyway…. But that
> requires more work in other parts of serf first. (I can’t use the request as
> an argument while serf still thinks it can destroy and recreate requests at
> a different address at will as part of the auth handling)
>
>
Ack.
--
Ivan Zhakov
Re: svn commit: r1715228 -/subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 20 November 2015 at 11:53, Bert Huijben <be...@qqmail.nl> wrote:
> Ack.
>
> Not pipelining, or not sending simultaneous/parallel requests if you don’t
> want to depend on the order in which they are received is the safest thing.
>
> And the current code can break even in http/1. Svnrdump even has special
> precautions for this even though I have no idea why it would see the
> problems it describes it has. (The editor drive is 100% from one http
> response)
>
> What we could do is replace the current assertion with a request on a
> completely different connection to retrieve the properties if we don’t have
> them in time… as a fallback mechanism to at least continue going. This also
> works for legacy servers if the first improvement is applied.
>
> I’m not sure if the current implementation has more problems… E.g. if
> revisions can also be received out of order.
>
> Going to a single report –that includes the revprops- for multiple revisions
> is a safe extension, that will improve in all cases: HTTP and HTTP/2 alike.
>
I understand that current is also unsafe, that's why I suggest go
single REPORT and disable pipeling for older servers. I'll add this to
my TODO list you agree that this approach makes sense.
> I will start looking in supporting priority scheduling anyway…. But that
> requires more work in other parts of serf first. (I can’t use the request as
> an argument while serf still thinks it can destroy and recreate requests at
> a different address at will as part of the auth handling)
>
>
Ack.
--
Ivan Zhakov
RE: svn commit: r1715228
-/subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
Ack.
Not pipelining, or not sending simultaneous/parallel requests if you don’t want to depend on the order in which they are received is the safest thing.
And the current code can break even in http/1. Svnrdump even has special precautions for this even though I have no idea why it would see the problems it describes it has. (The editor drive is 100% from one http response)
What we could do is replace the current assertion with a request on a completely different connection to retrieve the properties if we don’t have them in time… as a fallback mechanism to at least continue going. This also works for legacy servers if the first improvement is applied.
I’m not sure if the current implementation has more problems… E.g. if revisions can also be received out of order.
Going to a single report –that includes the revprops- for multiple revisions is a safe extension, that will improve in all cases: HTTP and HTTP/2 alike.
I will start looking in supporting priority scheduling anyway…. But that requires more work in other parts of serf first. (I can’t use the request as an argument while serf still thinks it can destroy and recreate requests at a different address at will as part of the auth handling)
Bert
Sent from Mail for Windows 10
From: Ivan Zhakov
Sent: vrijdag 20 november 2015 07:54
To: Bert Huijben
Cc: dev@subversion.apache.org
Subject: Re: svn commit: r1715228 -/subversion/trunk/subversion/libsvn_ra_serf/util.c
On 20 November 2015 at 01:48, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Bert Huijben [mailto:bert@qqmail.nl]
>> Sent: donderdag 19 november 2015 23:41
>> To: 'Ivan Zhakov' <iv...@visualsvn.com>
>> Cc: dev@subversion.apache.org
>> Subject: RE: svn commit: r1715228 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>>
>>
>> > -----Original Message-----
>> > From: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> > Sent: donderdag 19 november 2015 23:12
>> > To: Bert Huijben <be...@qqmail.nl>
>> > Cc: dev@subversion.apache.org
>> > Subject: Re: svn commit: r1715228 -
>> > /subversion/trunk/subversion/libsvn_ra_serf/util.c
>> >
>> > On 20 November 2015 at 00:03, Bert Huijben <be...@qqmail.nl> wrote:
>> > >> -----Original Message-----
>> > >> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
>> > >> Sent: donderdag 19 november 2015 19:03
>> > >> To: commits@subversion.apache.org
>> > >> Subject: svn commit: r1715228 -
>> > >> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>> > >>
>> > >> Author: rhuijben
>> > >> Date: Thu Nov 19 18:03:11 2015
>> > >> New Revision: 1715228
>> > >>
>> > >> URL: http://svn.apache.org/viewvc?rev=1715228&view=rev
>> > >> Log:
>> > >> Add a tiny bit of code to allow testing with Apache Serf's http/2 support.
>> > >>
>> > >> I committed this patch to celebrate that I got through basic_tests.py
>> > >> using the current http/2 support.
>> > >>
>> > >> * subversion/libsvn_ra_serf/util.c
>> > >> (conn_negotiate_protocol): New function.
>> > >> (conn_setup): Register usage of conn_negotiate_protocol when
>> > >> a very recent (read: trunk) serf + SVN__SERF_TEST_HTTP2 are used.
>> > >
>> > > Currently most tests pass for me over http2 when running with some
>> minor
>> > tweaks. I still get some aborted connections that aren't retried as cleanly as
>> > with http 1.1. Not sure what causes these yet; but this could also be an
>> httpd
>> > issues.
>> > >
>> > > The only ra function that really causes problems is the implementation of
>> > the replay
>> > > range api. This 100% expects that results will be received in the same
>> order
>> > in which
>> > > they are sent. This is typically the correct way in http 1.1 using serf, but
>> > even there it
>> > > is sometimes possible to get results out of order. (Authentication can re-
>> > queue
>> > > requests... and sometimes authorization is necessary even when earlier
>> > requests
>> > > succeeded, depending on the auth scheme).
>> > >
>> > Can we use WINDOW_UPDATE frame to control flow of replay-revision
>> > response? In this case we can use spillbuf for buffering responses to
>> > avoid latency performance regressions. What do you think?
>>
>> In theory we could... we could set the initial window to 0 and then send a
>> window update later. Assuming that httpd does that efficiently, etc. etc.
>> (Some of these assumptions are most likely not true)
>>
>> But I doubt if it would be more efficient than just sending the request when
>> we need it... or perhaps asking the properties a revision earlier. (We can also
>> send the request, but just leave out the end of stream marker... Sending EOS
>> will then handle the request)
>
> The proper 2.0 way would be to make the streams completely dependent on each other via priority handling.
>
> But using this priority information is optional on the server side. And I doubt that
> we can trust this to always do the right thing even if httpd would implement this
> correctly today.
>
Yes, stream priorities is another option, but it's not reliable. We
still have to manage spillbuf as fallback etc.
I agree that current code could fail in some edge cases, like
re-authentication for some reason.
I think simple solution for all these kind of problems would be just
add new replay-range REPORT that will stream all revision range in one
REPORT. We already do this for blame in file-revs REPORT, so I don't
see problems here. For older servers we can just disable pipelining in
this case. What do you think?
--
Ivan Zhakov
RE: svn commit: r1715228
-/subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
Ack.
Not pipelining, or not sending simultaneous/parallel requests if you don’t want to depend on the order in which they are received is the safest thing.
And the current code can break even in http/1. Svnrdump even has special precautions for this even though I have no idea why it would see the problems it describes it has. (The editor drive is 100% from one http response)
What we could do is replace the current assertion with a request on a completely different connection to retrieve the properties if we don’t have them in time… as a fallback mechanism to at least continue going. This also works for legacy servers if the first improvement is applied.
I’m not sure if the current implementation has more problems… E.g. if revisions can also be received out of order.
Going to a single report –that includes the revprops- for multiple revisions is a safe extension, that will improve in all cases: HTTP and HTTP/2 alike.
I will start looking in supporting priority scheduling anyway…. But that requires more work in other parts of serf first. (I can’t use the request as an argument while serf still thinks it can destroy and recreate requests at a different address at will as part of the auth handling)
Bert
Sent from Mail for Windows 10
From: Ivan Zhakov
Sent: vrijdag 20 november 2015 07:54
To: Bert Huijben
Cc: dev@subversion.apache.org
Subject: Re: svn commit: r1715228 -/subversion/trunk/subversion/libsvn_ra_serf/util.c
On 20 November 2015 at 01:48, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Bert Huijben [mailto:bert@qqmail.nl]
>> Sent: donderdag 19 november 2015 23:41
>> To: 'Ivan Zhakov' <iv...@visualsvn.com>
>> Cc: dev@subversion.apache.org
>> Subject: RE: svn commit: r1715228 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>>
>>
>> > -----Original Message-----
>> > From: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> > Sent: donderdag 19 november 2015 23:12
>> > To: Bert Huijben <be...@qqmail.nl>
>> > Cc: dev@subversion.apache.org
>> > Subject: Re: svn commit: r1715228 -
>> > /subversion/trunk/subversion/libsvn_ra_serf/util.c
>> >
>> > On 20 November 2015 at 00:03, Bert Huijben <be...@qqmail.nl> wrote:
>> > >> -----Original Message-----
>> > >> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
>> > >> Sent: donderdag 19 november 2015 19:03
>> > >> To: commits@subversion.apache.org
>> > >> Subject: svn commit: r1715228 -
>> > >> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>> > >>
>> > >> Author: rhuijben
>> > >> Date: Thu Nov 19 18:03:11 2015
>> > >> New Revision: 1715228
>> > >>
>> > >> URL: http://svn.apache.org/viewvc?rev=1715228&view=rev
>> > >> Log:
>> > >> Add a tiny bit of code to allow testing with Apache Serf's http/2 support.
>> > >>
>> > >> I committed this patch to celebrate that I got through basic_tests.py
>> > >> using the current http/2 support.
>> > >>
>> > >> * subversion/libsvn_ra_serf/util.c
>> > >> (conn_negotiate_protocol): New function.
>> > >> (conn_setup): Register usage of conn_negotiate_protocol when
>> > >> a very recent (read: trunk) serf + SVN__SERF_TEST_HTTP2 are used.
>> > >
>> > > Currently most tests pass for me over http2 when running with some
>> minor
>> > tweaks. I still get some aborted connections that aren't retried as cleanly as
>> > with http 1.1. Not sure what causes these yet; but this could also be an
>> httpd
>> > issues.
>> > >
>> > > The only ra function that really causes problems is the implementation of
>> > the replay
>> > > range api. This 100% expects that results will be received in the same
>> order
>> > in which
>> > > they are sent. This is typically the correct way in http 1.1 using serf, but
>> > even there it
>> > > is sometimes possible to get results out of order. (Authentication can re-
>> > queue
>> > > requests... and sometimes authorization is necessary even when earlier
>> > requests
>> > > succeeded, depending on the auth scheme).
>> > >
>> > Can we use WINDOW_UPDATE frame to control flow of replay-revision
>> > response? In this case we can use spillbuf for buffering responses to
>> > avoid latency performance regressions. What do you think?
>>
>> In theory we could... we could set the initial window to 0 and then send a
>> window update later. Assuming that httpd does that efficiently, etc. etc.
>> (Some of these assumptions are most likely not true)
>>
>> But I doubt if it would be more efficient than just sending the request when
>> we need it... or perhaps asking the properties a revision earlier. (We can also
>> send the request, but just leave out the end of stream marker... Sending EOS
>> will then handle the request)
>
> The proper 2.0 way would be to make the streams completely dependent on each other via priority handling.
>
> But using this priority information is optional on the server side. And I doubt that
> we can trust this to always do the right thing even if httpd would implement this
> correctly today.
>
Yes, stream priorities is another option, but it's not reliable. We
still have to manage spillbuf as fallback etc.
I agree that current code could fail in some edge cases, like
re-authentication for some reason.
I think simple solution for all these kind of problems would be just
add new replay-range REPORT that will stream all revision range in one
REPORT. We already do this for blame in file-revs REPORT, so I don't
see problems here. For older servers we can just disable pipelining in
this case. What do you think?
--
Ivan Zhakov
Re: svn commit: r1715228 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 20 November 2015 at 01:48, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Bert Huijben [mailto:bert@qqmail.nl]
>> Sent: donderdag 19 november 2015 23:41
>> To: 'Ivan Zhakov' <iv...@visualsvn.com>
>> Cc: dev@subversion.apache.org
>> Subject: RE: svn commit: r1715228 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>>
>>
>> > -----Original Message-----
>> > From: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> > Sent: donderdag 19 november 2015 23:12
>> > To: Bert Huijben <be...@qqmail.nl>
>> > Cc: dev@subversion.apache.org
>> > Subject: Re: svn commit: r1715228 -
>> > /subversion/trunk/subversion/libsvn_ra_serf/util.c
>> >
>> > On 20 November 2015 at 00:03, Bert Huijben <be...@qqmail.nl> wrote:
>> > >> -----Original Message-----
>> > >> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
>> > >> Sent: donderdag 19 november 2015 19:03
>> > >> To: commits@subversion.apache.org
>> > >> Subject: svn commit: r1715228 -
>> > >> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>> > >>
>> > >> Author: rhuijben
>> > >> Date: Thu Nov 19 18:03:11 2015
>> > >> New Revision: 1715228
>> > >>
>> > >> URL: http://svn.apache.org/viewvc?rev=1715228&view=rev
>> > >> Log:
>> > >> Add a tiny bit of code to allow testing with Apache Serf's http/2 support.
>> > >>
>> > >> I committed this patch to celebrate that I got through basic_tests.py
>> > >> using the current http/2 support.
>> > >>
>> > >> * subversion/libsvn_ra_serf/util.c
>> > >> (conn_negotiate_protocol): New function.
>> > >> (conn_setup): Register usage of conn_negotiate_protocol when
>> > >> a very recent (read: trunk) serf + SVN__SERF_TEST_HTTP2 are used.
>> > >
>> > > Currently most tests pass for me over http2 when running with some
>> minor
>> > tweaks. I still get some aborted connections that aren't retried as cleanly as
>> > with http 1.1. Not sure what causes these yet; but this could also be an
>> httpd
>> > issues.
>> > >
>> > > The only ra function that really causes problems is the implementation of
>> > the replay
>> > > range api. This 100% expects that results will be received in the same
>> order
>> > in which
>> > > they are sent. This is typically the correct way in http 1.1 using serf, but
>> > even there it
>> > > is sometimes possible to get results out of order. (Authentication can re-
>> > queue
>> > > requests... and sometimes authorization is necessary even when earlier
>> > requests
>> > > succeeded, depending on the auth scheme).
>> > >
>> > Can we use WINDOW_UPDATE frame to control flow of replay-revision
>> > response? In this case we can use spillbuf for buffering responses to
>> > avoid latency performance regressions. What do you think?
>>
>> In theory we could... we could set the initial window to 0 and then send a
>> window update later. Assuming that httpd does that efficiently, etc. etc.
>> (Some of these assumptions are most likely not true)
>>
>> But I doubt if it would be more efficient than just sending the request when
>> we need it... or perhaps asking the properties a revision earlier. (We can also
>> send the request, but just leave out the end of stream marker... Sending EOS
>> will then handle the request)
>
> The proper 2.0 way would be to make the streams completely dependent on each other via priority handling.
>
> But using this priority information is optional on the server side. And I doubt that
> we can trust this to always do the right thing even if httpd would implement this
> correctly today.
>
Yes, stream priorities is another option, but it's not reliable. We
still have to manage spillbuf as fallback etc.
I agree that current code could fail in some edge cases, like
re-authentication for some reason.
I think simple solution for all these kind of problems would be just
add new replay-range REPORT that will stream all revision range in one
REPORT. We already do this for blame in file-revs REPORT, so I don't
see problems here. For older servers we can just disable pipelining in
this case. What do you think?
--
Ivan Zhakov
RE: svn commit: r1715228 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: donderdag 19 november 2015 23:41
> To: 'Ivan Zhakov' <iv...@visualsvn.com>
> Cc: dev@subversion.apache.org
> Subject: RE: svn commit: r1715228 -
> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>
>
>
> > -----Original Message-----
> > From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> > Sent: donderdag 19 november 2015 23:12
> > To: Bert Huijben <be...@qqmail.nl>
> > Cc: dev@subversion.apache.org
> > Subject: Re: svn commit: r1715228 -
> > /subversion/trunk/subversion/libsvn_ra_serf/util.c
> >
> > On 20 November 2015 at 00:03, Bert Huijben <be...@qqmail.nl> wrote:
> > >> -----Original Message-----
> > >> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
> > >> Sent: donderdag 19 november 2015 19:03
> > >> To: commits@subversion.apache.org
> > >> Subject: svn commit: r1715228 -
> > >> /subversion/trunk/subversion/libsvn_ra_serf/util.c
> > >>
> > >> Author: rhuijben
> > >> Date: Thu Nov 19 18:03:11 2015
> > >> New Revision: 1715228
> > >>
> > >> URL: http://svn.apache.org/viewvc?rev=1715228&view=rev
> > >> Log:
> > >> Add a tiny bit of code to allow testing with Apache Serf's http/2 support.
> > >>
> > >> I committed this patch to celebrate that I got through basic_tests.py
> > >> using the current http/2 support.
> > >>
> > >> * subversion/libsvn_ra_serf/util.c
> > >> (conn_negotiate_protocol): New function.
> > >> (conn_setup): Register usage of conn_negotiate_protocol when
> > >> a very recent (read: trunk) serf + SVN__SERF_TEST_HTTP2 are used.
> > >
> > > Currently most tests pass for me over http2 when running with some
> minor
> > tweaks. I still get some aborted connections that aren't retried as cleanly as
> > with http 1.1. Not sure what causes these yet; but this could also be an
> httpd
> > issues.
> > >
> > > The only ra function that really causes problems is the implementation of
> > the replay
> > > range api. This 100% expects that results will be received in the same
> order
> > in which
> > > they are sent. This is typically the correct way in http 1.1 using serf, but
> > even there it
> > > is sometimes possible to get results out of order. (Authentication can re-
> > queue
> > > requests... and sometimes authorization is necessary even when earlier
> > requests
> > > succeeded, depending on the auth scheme).
> > >
> > Can we use WINDOW_UPDATE frame to control flow of replay-revision
> > response? In this case we can use spillbuf for buffering responses to
> > avoid latency performance regressions. What do you think?
>
> In theory we could... we could set the initial window to 0 and then send a
> window update later. Assuming that httpd does that efficiently, etc. etc.
> (Some of these assumptions are most likely not true)
>
> But I doubt if it would be more efficient than just sending the request when
> we need it... or perhaps asking the properties a revision earlier. (We can also
> send the request, but just leave out the end of stream marker... Sending EOS
> will then handle the request)
The proper 2.0 way would be to make the streams completely dependent on each other via priority handling.
But using this priority information is optional on the server side. And I doubt that we can trust this to always do the right thing even if httpd would implement this correctly today.
Bert
RE: svn commit: r1715228 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: donderdag 19 november 2015 23:12
> To: Bert Huijben <be...@qqmail.nl>
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r1715228 -
> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>
> On 20 November 2015 at 00:03, Bert Huijben <be...@qqmail.nl> wrote:
> >> -----Original Message-----
> >> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
> >> Sent: donderdag 19 november 2015 19:03
> >> To: commits@subversion.apache.org
> >> Subject: svn commit: r1715228 -
> >> /subversion/trunk/subversion/libsvn_ra_serf/util.c
> >>
> >> Author: rhuijben
> >> Date: Thu Nov 19 18:03:11 2015
> >> New Revision: 1715228
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1715228&view=rev
> >> Log:
> >> Add a tiny bit of code to allow testing with Apache Serf's http/2 support.
> >>
> >> I committed this patch to celebrate that I got through basic_tests.py
> >> using the current http/2 support.
> >>
> >> * subversion/libsvn_ra_serf/util.c
> >> (conn_negotiate_protocol): New function.
> >> (conn_setup): Register usage of conn_negotiate_protocol when
> >> a very recent (read: trunk) serf + SVN__SERF_TEST_HTTP2 are used.
> >
> > Currently most tests pass for me over http2 when running with some minor
> tweaks. I still get some aborted connections that aren't retried as cleanly as
> with http 1.1. Not sure what causes these yet; but this could also be an httpd
> issues.
> >
> > The only ra function that really causes problems is the implementation of
> the replay
> > range api. This 100% expects that results will be received in the same order
> in which
> > they are sent. This is typically the correct way in http 1.1 using serf, but
> even there it
> > is sometimes possible to get results out of order. (Authentication can re-
> queue
> > requests... and sometimes authorization is necessary even when earlier
> requests
> > succeeded, depending on the auth scheme).
> >
> Can we use WINDOW_UPDATE frame to control flow of replay-revision
> response? In this case we can use spillbuf for buffering responses to
> avoid latency performance regressions. What do you think?
In theory we could... we could set the initial window to 0 and then send a window update later. Assuming that httpd does that efficiently, etc. etc. (Some of these assumptions are most likely not true)
But I doubt if it would be more efficient than just sending the request when we need it... or perhaps asking the properties a revision earlier. (We can also send the request, but just leave out the end of stream marker... Sending EOS will then handle the request)
The http/1 style just send all the requests early just doesn't work out in case we want to handle them in strict order. In this very specific case it might even be more efficient to just use http/1.1. The way we currently want to handle it, is really like one big bulk request.
I remember that we even disabled pipelining in some of these cases (perhaps we still do).... This really needs a better design on the Subversion side.
Bert
Re: svn commit: r1715228 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 20 November 2015 at 00:03, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
>> Sent: donderdag 19 november 2015 19:03
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1715228 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>> Author: rhuijben
>> Date: Thu Nov 19 18:03:11 2015
>> New Revision: 1715228
>>
>> URL: http://svn.apache.org/viewvc?rev=1715228&view=rev
>> Log:
>> Add a tiny bit of code to allow testing with Apache Serf's http/2 support.
>>
>> I committed this patch to celebrate that I got through basic_tests.py
>> using the current http/2 support.
>>
>> * subversion/libsvn_ra_serf/util.c
>> (conn_negotiate_protocol): New function.
>> (conn_setup): Register usage of conn_negotiate_protocol when
>> a very recent (read: trunk) serf + SVN__SERF_TEST_HTTP2 are used.
>
> Currently most tests pass for me over http2 when running with some minor tweaks. I still get some aborted connections that aren't retried as cleanly as with http 1.1. Not sure what causes these yet; but this could also be an httpd issues.
>
> The only ra function that really causes problems is the implementation of the replay
> range api. This 100% expects that results will be received in the same order in which
> they are sent. This is typically the correct way in http 1.1 using serf, but even there it
> is sometimes possible to get results out of order. (Authentication can re-queue
> requests... and sometimes authorization is necessary even when earlier requests
> succeeded, depending on the auth scheme).
>
Can we use WINDOW_UPDATE frame to control flow of replay-revision
response? In this case we can use spillbuf for buffering responses to
avoid latency performance regressions. What do you think?
--
Ivan Zhakov
RE: svn commit: r1715228 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
> Sent: donderdag 19 november 2015 19:03
> To: commits@subversion.apache.org
> Subject: svn commit: r1715228 -
> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>
> Author: rhuijben
> Date: Thu Nov 19 18:03:11 2015
> New Revision: 1715228
>
> URL: http://svn.apache.org/viewvc?rev=1715228&view=rev
> Log:
> Add a tiny bit of code to allow testing with Apache Serf's http/2 support.
>
> I committed this patch to celebrate that I got through basic_tests.py
> using the current http/2 support.
>
> * subversion/libsvn_ra_serf/util.c
> (conn_negotiate_protocol): New function.
> (conn_setup): Register usage of conn_negotiate_protocol when
> a very recent (read: trunk) serf + SVN__SERF_TEST_HTTP2 are used.
Currently most tests pass for me over http2 when running with some minor tweaks. I still get some aborted connections that aren't retried as cleanly as with http 1.1. Not sure what causes these yet; but this could also be an httpd issues.
The only ra function that really causes problems is the implementation of the replay range api. This 100% expects that results will be received in the same order in which they are sent. This is typically the correct way in http 1.1 using serf, but even there it is sometimes possible to get results out of order. (Authentication can re-queue requests... and sometimes authorization is necessary even when earlier requests succeeded, depending on the auth scheme).
If you want to try this for yourself, you need http 2.4.17 with mod_http2 (or newer) and OpenSSL 1.0.2 or newer. (If you patch Subversion a bit further you should be able to use h2c, which avoids the openssl dependency).
Bert