RE: svn commit: r1715228 -/subversion/trunk/subversion/libsvn_ra_serf/util.c

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_at_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 <bert_at_qqmail.nl> wrote:
>> -----Original Message-----
>> From: Bert Huijben [mailto:bert_at_qqmail.nl]
>> Sent: donderdag 19 november 2015 23:41
>> To: 'Ivan Zhakov' <ivan_at_visualsvn.com>
>> Cc: dev_at_subversion.apache.org
>> Subject: RE: svn commit: r1715228 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>>
>>
>> > -----Original Message-----
>> > From: Ivan Zhakov [mailto:ivan_at_visualsvn.com]
>> > Sent: donderdag 19 november 2015 23:12
>> > To: Bert Huijben <bert_at_qqmail.nl>
>> > Cc: dev_at_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 <bert_at_qqmail.nl> wrote:
>> > >> -----Original Message-----
>> > >> From: rhuijben_at_apache.org [mailto:rhuijben_at_apache.org]
>> > >> Sent: donderdag 19 november 2015 19:03
>> > >> To: commits_at_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