You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2014/01/07 15:29:26 UTC
RE: svn commit: r1555716
-/subversion/trunk/subversion/libsvn_ra_serf/update.c
I used 128, 256 but didn't get a problem. The same values might also hit a corner case in the spill buffer. Will look at it in a few minutes.
Bert
-----Original Message-----
From: "Ivan Zhakov" <iv...@visualsvn.com>
Sent: 7-1-2014 13:55
To: "Bert Huijben" <be...@qqmail.nl>
Cc: "Subversion Development" <de...@subversion.apache.org>
Subject: Re: svn commit: r1555716 -/subversion/trunk/subversion/libsvn_ra_serf/update.c
On 7 January 2014 12:42, Bert Huijben <be...@qqmail.nl> wrote:
> The current code works in both scenarios (tested via buffersizes of just a
> few bytes, and a debugger verifyimg the corner cases, etc). But I doubt it
> is worth all the trouble. The file is null until something is spilled and
> from that point onwards everything is in the file (until the spill is
> empty).
>
Few bytes is bad test because memory isn't used at all in this case.
I've tested and code doesn't work as expected. I've tested following
patch to update subversion 1.8.x working copy:
[[[
Index: subversion/libsvn_ra_serf/update.c
===================================================================
--- subversion/libsvn_ra_serf/update.c (revision 1556193)
+++ subversion/libsvn_ra_serf/update.c (working copy)
@@ -3429,7 +3429,7 @@
*reporter = &ra_serf_reporter;
*report_baton = report;
- report->body_sb = svn_spillbuf__create(1024, 131072, report->pool);
+ report->body_sb = svn_spillbuf__create(220, 220, report->pool);
if (sess->bulk_updates == svn_tristate_true)
{
]]]
It fails with:
[[[
..\..\..\subversion\svn\update-cmd.c:172,
..\..\..\subversion\libsvn_client\update.c:722,
..\..\..\subversion\libsvn_client\update.c:614,
..\..\..\subversion\libsvn_client\update.c:466,
..\..\..\subversion\libsvn_wc\adm_crawler.c:844,
..\..\..\subversion\libsvn_ra_serf\update.c:3131,
..\..\..\subversion\libsvn_ra_serf\util.c:915,
..\..\..\subversion\libsvn_ra_serf\util.c:2270,
..\..\..\subversion\libsvn_ra_serf\util.c:2043,
..\..\..\subversion\libsvn_ra_serf\util.c:2030:
(apr_err=SVN_ERR_RA_DAV_REQUEST_FAILED)
svn: E175002: Unexpected HTTP status 400 'Bad Request' on '/repos/asf/!svn/me'
]]]
> If you want to look at improving it now, feel free. I'm first working on
> switching the update logic to the new xml parser. (And will perform a
> further cleanup including these points round after that).
>
I suggest to remove this optimized code path completely: potential
optimization is not significant imho. Also such big update request
bodies are not usual. I can remove this code if you're busy with other
stuff.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
RE: svn commit: r1555716 -/subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: donderdag 9 januari 2014 10:18
> To: Bert Huijben
> Cc: Subversion Development
> Subject: Re: svn commit: r1555716 -
> /subversion/trunk/subversion/libsvn_ra_serf/update.c
>
> On 7 January 2014 18:29, Bert Huijben <be...@qqmail.nl> wrote:
> > On 7 January 2014 12:42, Bert Huijben <be...@qqmail.nl> wrote:
> >> The current code works in both scenarios (tested via buffersizes of just a
> >> few bytes, and a debugger verifyimg the corner cases, etc). But I doubt it
> >> is worth all the trouble. The file is null until something is spilled and
> >> from that point onwards everything is in the file (until the spill is
> >> empty).
> >>
> > Few bytes is bad test because memory isn't used at all in this case.
> >
> > I've tested and code doesn't work as expected. > I used 128, 256 but didn't
> get a problem. The same values might also hit a
> > corner case in the spill buffer. Will look at it in a few minutes.
> >
> Hi Bert.
>
> I see you already removed this optimization in r1556343. Thanks.
>
> But it seems there is another problem with using spillbuf for request
> bodies: create_update_report_body() callback can be called multiple
> times if request need to be resend because of authentication challenge
> or KeepAlive limit. In this case you will get empty request body on
> second invocation, because all spillbuf is already read.
>
> I see two options how to fix this:
> 1. Do not use spillbuf for request bodies. Use custom implementation
> based on stringbuf and temporary file if needed.
> 2. Implement svn_spillbuf__rewind() or something.
Wow... good catch. I'm surprised that we only find this now and not much shorter after I committed this. (Shouldn't we have tests for a simple authenticated scenario... or... well maybe the OPTIONS pipelining handles the simple cases?)
Instead of a rewind we could also use something like a 'peek' feature here (like on the serf buckets), which would keep us closer to the original intent.
Bert
Re: svn commit: r1555716 -/subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Branko Čibej <br...@wandisco.com>.
On 09.01.2014 10:38, Ivan Zhakov wrote:
> On 9 January 2014 13:29, Branko Čibej <br...@wandisco.com> wrote:
>> On 09.01.2014 10:18, Ivan Zhakov wrote:
>>
>> On 7 January 2014 18:29, Bert Huijben <be...@qqmail.nl> wrote:
>>
>> On 7 January 2014 12:42, Bert Huijben <be...@qqmail.nl> wrote:
>>
>> The current code works in both scenarios (tested via buffersizes of just a
>> few bytes, and a debugger verifyimg the corner cases, etc). But I doubt it
>> is worth all the trouble. The file is null until something is spilled and
>> from that point onwards everything is in the file (until the spill is
>> empty).
>>
>> Few bytes is bad test because memory isn't used at all in this case.
>>
>> I've tested and code doesn't work as expected. > I used 128, 256 but didn't
>> get a problem. The same values might also hit a
>> corner case in the spill buffer. Will look at it in a few minutes.
>>
>> Hi Bert.
>>
>> I see you already removed this optimization in r1556343. Thanks.
>>
>> But it seems there is another problem with using spillbuf for request
>> bodies: create_update_report_body() callback can be called multiple
>> times if request need to be resend because of authentication challenge
>> or KeepAlive limit. In this case you will get empty request body on
>> second invocation, because all spillbuf is already read.
>>
>> I see two options how to fix this:
>> 1. Do not use spillbuf for request bodies. Use custom implementation
>> based on stringbuf and temporary file if needed.
>> 2. Implement svn_spillbuf__rewind() or something.
>>
>> What do you think?
>>
>>
>> Definitely implement rewind, IMO.
>>
> I'm not sure, because as far I remember orignally spillbuf was
> implemented with queue/circular buffer behavior. But then spillbuf was
> extended for other use cases, so may be there is no problem with
> svn_spillbuf__rewind().
You're right about the original intent. The extensions for other use
cases are not used anywhere at the moment, fwiw; my idea was to use the
underlying common functionality for compressed pristines, but I never
got around to getting any further than that.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1555716 -/subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 9 January 2014 13:29, Branko Čibej <br...@wandisco.com> wrote:
> On 09.01.2014 10:18, Ivan Zhakov wrote:
>
> On 7 January 2014 18:29, Bert Huijben <be...@qqmail.nl> wrote:
>
> On 7 January 2014 12:42, Bert Huijben <be...@qqmail.nl> wrote:
>
> The current code works in both scenarios (tested via buffersizes of just a
> few bytes, and a debugger verifyimg the corner cases, etc). But I doubt it
> is worth all the trouble. The file is null until something is spilled and
> from that point onwards everything is in the file (until the spill is
> empty).
>
> Few bytes is bad test because memory isn't used at all in this case.
>
> I've tested and code doesn't work as expected. > I used 128, 256 but didn't
> get a problem. The same values might also hit a
> corner case in the spill buffer. Will look at it in a few minutes.
>
> Hi Bert.
>
> I see you already removed this optimization in r1556343. Thanks.
>
> But it seems there is another problem with using spillbuf for request
> bodies: create_update_report_body() callback can be called multiple
> times if request need to be resend because of authentication challenge
> or KeepAlive limit. In this case you will get empty request body on
> second invocation, because all spillbuf is already read.
>
> I see two options how to fix this:
> 1. Do not use spillbuf for request bodies. Use custom implementation
> based on stringbuf and temporary file if needed.
> 2. Implement svn_spillbuf__rewind() or something.
>
> What do you think?
>
>
> Definitely implement rewind, IMO.
>
I'm not sure, because as far I remember orignally spillbuf was
implemented with queue/circular buffer behavior. But then spillbuf was
extended for other use cases, so may be there is no problem with
svn_spillbuf__rewind().
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1555716 -/subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Branko Čibej <br...@wandisco.com>.
On 09.01.2014 10:18, Ivan Zhakov wrote:
> On 7 January 2014 18:29, Bert Huijben <be...@qqmail.nl> wrote:
>> On 7 January 2014 12:42, Bert Huijben <be...@qqmail.nl> wrote:
>>> The current code works in both scenarios (tested via buffersizes of just a
>>> few bytes, and a debugger verifyimg the corner cases, etc). But I doubt it
>>> is worth all the trouble. The file is null until something is spilled and
>>> from that point onwards everything is in the file (until the spill is
>>> empty).
>>>
>> Few bytes is bad test because memory isn't used at all in this case.
>>
>> I've tested and code doesn't work as expected. > I used 128, 256 but didn't get a problem. The same values might also hit a
>> corner case in the spill buffer. Will look at it in a few minutes.
>>
> Hi Bert.
>
> I see you already removed this optimization in r1556343. Thanks.
>
> But it seems there is another problem with using spillbuf for request
> bodies: create_update_report_body() callback can be called multiple
> times if request need to be resend because of authentication challenge
> or KeepAlive limit. In this case you will get empty request body on
> second invocation, because all spillbuf is already read.
>
> I see two options how to fix this:
> 1. Do not use spillbuf for request bodies. Use custom implementation
> based on stringbuf and temporary file if needed.
> 2. Implement svn_spillbuf__rewind() or something.
>
> What do you think?
Definitely implement rewind, IMO.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1555716 -/subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 January 2014 18:29, Bert Huijben <be...@qqmail.nl> wrote:
> On 7 January 2014 12:42, Bert Huijben <be...@qqmail.nl> wrote:
>> The current code works in both scenarios (tested via buffersizes of just a
>> few bytes, and a debugger verifyimg the corner cases, etc). But I doubt it
>> is worth all the trouble. The file is null until something is spilled and
>> from that point onwards everything is in the file (until the spill is
>> empty).
>>
> Few bytes is bad test because memory isn't used at all in this case.
>
> I've tested and code doesn't work as expected. > I used 128, 256 but didn't get a problem. The same values might also hit a
> corner case in the spill buffer. Will look at it in a few minutes.
>
Hi Bert.
I see you already removed this optimization in r1556343. Thanks.
But it seems there is another problem with using spillbuf for request
bodies: create_update_report_body() callback can be called multiple
times if request need to be resend because of authentication challenge
or KeepAlive limit. In this case you will get empty request body on
second invocation, because all spillbuf is already read.
I see two options how to fix this:
1. Do not use spillbuf for request bodies. Use custom implementation
based on stringbuf and temporary file if needed.
2. Implement svn_spillbuf__rewind() or something.
What do you think?
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com